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

Remove create_new_model_version arg of ModelConfig #2030

Merged
merged 15 commits into from
Nov 10, 2023

Conversation

avishniakov
Copy link
Contributor

@avishniakov avishniakov commented Nov 7, 2023

Describe changes

I simplified the ModelConfig interface to avoid explicitly passing around create_new_model_version.
New behavior expects that the user keeps version==None if the new unnamed version is needed, otherwise model version will be retrieved/created based on the version argument.
Also new ModelStages.LATEST was introduced in order to give the ability to fetch the LATEST model version.

[BREAKING CHANGE] Old configurations using create_new_model_version might lead to unpredicted results, but the change needed is simple:

ModelConfig(name="foo",create_new_model_version=True) -> ModelConfig(name="foo")
ModelConfig(name="foo", version="bar", create_new_model_version=True) -> ModelConfig(name="foo", version="bar")
ModelConfig(name="foo", version="bar", create_new_model_version=False) -> ModelConfig(name="foo", version="bar")
ModelConfig(name="foo", version=None, create_new_model_version=False) -> ModelConfig(name="foo", version=ModelStages.LATEST)

[BEHAVIOR CHANGE] If a signature like ModelConfig(name="foo", version="bar", create_new_model_version=True) was used, it prevented 2 parallel pipelines from running pointing to same model version, this is not the case anymore for named versions.

Tests updates mostly relate to signature updates, but also some tests were removed as no longer valid ones.

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • If my change requires a change to docs, I have updated the documentation accordingly.
  • If I have added an integration, I have updated the integrations table and the corresponding website section.
  • I have added tests to cover my changes.
  • I have based my new branch on develop and the open PR is targeting develop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.

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)

@avishniakov avishniakov added breaking-change enhancement New feature or request internal To filter out internal PRs and issues labels Nov 7, 2023
@avishniakov avishniakov requested review from fa9r and strickvl November 7, 2023 16:13
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.

The main change looks good but I'm not a fan of the changes to the client, models, and zenstore methods. For these, fetching the latest version by default made sense and was more convenient to use with the version arg being optional. I'd suggest reverting these changes as indicated in the comments below, otherwise LGTM.

src/zenml/client.py Outdated Show resolved Hide resolved
src/zenml/models/model_models.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/zen_store_interface.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
@@ -307,3 +260,22 @@ def _merge(self, model_config: "ModelConfig") -> None:
self.delete_new_version_on_failure &= (
model_config.delete_new_version_on_failure
)

def __hash__(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avishniakov avishniakov requested a review from fa9r November 10, 2023 11:55
@avishniakov
Copy link
Contributor Author

@fa9r , tests passed, if you don't see any more changes needed here, I would love to merge this to proceed with renaming ticket.

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.

LGTM

@avishniakov avishniakov merged commit 55f4306 into develop Nov 10, 2023
@avishniakov avishniakov deleted the feature/OSS-2608-deprecate-creates_new_version branch November 10, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change 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.

2 participants