-
Notifications
You must be signed in to change notification settings - Fork 454
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
ModelConfig
is ModelVersion
!
#2044
ModelConfig
is ModelVersion
!
#2044
Conversation
…' of https://github.com/zenml-io/zenml into feature/OSS-2609-OSS-2575-model-config-is-model-version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall changes look good, just needs a bit more polish before merging 👍
@@ -375,7 +368,7 @@ def list_model_versions(model_name_or_id: str, **kwargs: Any) -> None: | |||
**kwargs: Keyword arguments to filter models. | |||
""" | |||
model_id = Client().get_model(model_name_or_id=model_name_or_id).id | |||
model_versions = Client().list_model_versions( | |||
model_versions = Client().zen_store.list_model_versions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we bypassing the client method here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I need the response model back, not ModelVersion
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested in the other comment - the Client should return the response model anyway if we want to be consistent with the current Client interface, so I guess this would also solve this issue then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to keep it as is, no one even knows about zenml.models.model_models.ModelVersionResponseModel thing - it is too internal to be a concerning, in my opinion.
E2E template updates in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only major issue I see is that the Client methods are still inconsistent with other entities, other than that it looks ready to merge from my side
…' of https://github.com/zenml-io/zenml into feature/OSS-2609-OSS-2575-model-config-is-model-version
* Moved model versions to their own root route * Fix the delete endpoint * Fixed smaller mistakes * Fixed for Client as well * Fixed lint errors * More linting * More linting * More tiny fixes * Update src/zenml/zen_server/routers/model_versions_endpoints.py Co-authored-by: Andrei Vishniakov <[email protected]> * More linting * Fixed tests and solved conflicts * Fixed linting * Fixed more tests * Further refactoring * Added raises section * Fix one failing test * Take "latest" stage into account * Reformatted * Standardize use of list response * Rewrote some tests * Add clien tests * Fixed spelling * Tested to work with e2e pipeline * Ugly fixes to get response models in the CLI * Auto-update of E2E template * Access ModelVersionResponseModels in Client again * Another small fix * Linted --------- Co-authored-by: Andrei Vishniakov <[email protected]> Co-authored-by: GitHub Actions <[email protected]>
E2E template updates in |
@fa9r , any vetos merging this? Otherwise, I'll merge. |
This reverts commit b518dcc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still suggest to change create / get / update to return ModelVersionResponseModel
again, otherwise LGTM
stage: Optional[Union[str, ModelStages]] = None, | ||
force: bool = False, | ||
name: Optional[str] = None, | ||
) -> "ModelVersion": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not a fan of this returning ModelVersion
instead of ModelVersionResponseModel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's proceed like this then:
- I will merge this one to unblock others (including you with rebasing your mega-branch).
- Spin a feedback loop on Discord to clarify what we should return and if we should stick to responses here.
- If we decide to use responses - I'll raise a separate ticket for that purpose.
Describe changes
This is a big refactoring PR containing many renames and breaking changes. The ultimate goal was to bring a simplified user-interface experience, so once
ModelVersion
is configured in@pipeline
or@step
it will travel into all other user-facing places: step context, client, etc.[BREKING CHANGES]
ModelConfig
->ModelVersion
@pipeline(model_config=...)
->@pipeline(model_version=...)
model_config
->model_version
create_model_version
,get_model_version
andupdate_model_version
now returnModelVersion
get_step_context().model_version
now returnModelVersion
model_objects
>model_artifacts
,artifacts
>data_artifacts
anddeployments
>endpoint_artifacts
in CLI and other places in code.get_or_create_model_version
andget_or_create_model
are now private and called under the hood of other functions, so meant to be non-user-facing going forwardArtifactConfig
->DataArtifactConfig
DeploymentArtifactConfig
->EndpointArtifactConfig
running
model versions and surrounding complexity for users. Now if the version is None in model_version a new model version will get created and shared across all other model_version requests inside the same run.DataArtifactConfig
and others are no longer working with onlymodel_name
-model_version
needs to be fed along. In case of misuse, a clear RuntimeError will be raised. These objects can still be used withoutmodel_name
andmodel_version
to get context from the step.BUGFIXES
ModelVersion(...,version="staging")
in some stepsSIDE CHANGES
ModelVersion
is now equipped with extra public methods, which I find useful. Have a look, if something is missing in your opinion.Incoming templates PR: zenml-io/template-e2e-batch#19
Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes