Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase reuse of ModelConfig #1954

Conversation

avishniakov
Copy link
Contributor

@avishniakov avishniakov commented Oct 17, 2023

Describe changes

This PR removes ModelConfigModel; all codes now use ModelConfig instead. ModelConfig now has a model and model_version to avoid multiple calls to DB while working inside the same context.
ModelConfig is still instantiated on every step due to how reading of configs from DB works currently - it gets a dict and converts to_model where applicable, but no extra calls to DB are done with the current architecture.

Pre-requisites

Please ensure you have done the following:

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (add details above)

@github-actions github-actions bot added internal To filter out internal PRs and issues enhancement New feature or request labels Oct 17, 2023
@avishniakov avishniakov changed the title Increase reuse ofModelConfig Increase reuse of ModelConfig Oct 17, 2023
@avishniakov avishniakov marked this pull request as ready for review October 17, 2023 12:45
@avishniakov avishniakov marked this pull request as draft October 17, 2023 12:49
src/zenml/model/model_config.py Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
Copy link
Contributor

@fa9r fa9r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this feels much cleaner 🙌

Just have a few more minor suggestions, otherwise this looks good to merge 👍

src/zenml/new/pipelines/pipeline.py Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
src/zenml/model/model_config.py Outdated Show resolved Hide resolved
src/zenml/cli/model.py Outdated Show resolved Hide resolved
@@ -515,7 +521,7 @@ def _print_artifacts_links_generic(
help="List artifacts linked to a model version.",
)
@click.argument("model_name_or_id")
@click.argument("model_version_name_or_number_or_id")
@click.argument("model_version_name_or_number_or_id", default="0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit weird, why can't we use None as default / latest value instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cause it is strings - cli arguments and -1 which I planned to use is treated as a parameter name, so 0 seemed a solid choice. Moreover - user will not specify it directly, it will be set by just skipping this arg.
zenml model version artifacts my_model -> will return artifacts for latest in my_model

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it, why can't it be Optional[str] instead? I.e., why would this not work?

@click.argument("model_name_or_id")
@click.argument("model_version_name_or_number_or_id")
def list_model_version_artifacts(
    model_name_or_id: str,
    model_version_name_or_number_or_id: Optional[str] = None,
) -> None:
    ...

def _print_artifacts_links_generic(
    model_name_or_id: str,
    model_version_name_or_number_or_id: Optional[str] = None,
    ...
):
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a click limitation, as I see this. If I set default=None it is treated as no default, making arg mandatory. Changes on the level of functions args are not effective at all - @click.argument is the king here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, click.argument means mandatory, for optional inputs you need to use click.option

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, the option would lead to the argument passing as --model_version_name_or_number_or_id and not just zenml some_command some_argument [some_skipped_optional_argument] or I completely misuse click 🙂

tests/unit/model/test_model_config_init.py Show resolved Hide resolved
@avishniakov avishniakov marked this pull request as draft October 18, 2023 14:20
@avishniakov avishniakov marked this pull request as ready for review October 18, 2023 14:41
Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great + lovely to have it well tested, too.

@avishniakov avishniakov merged commit 7a52882 into develop Oct 19, 2023
37 checks passed
@avishniakov avishniakov deleted the feature/OSS-2510-reduce-the-number-of-re-instantiations-of-objects-in-model-co branch October 19, 2023 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internal To filter out internal PRs and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants