-
Notifications
You must be signed in to change notification settings - Fork 461
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
Artifacts Tab #1943
Artifacts Tab #1943
Conversation
* [Artifacts Tab] Globally Unique Artifact Names * Fix default names for unlisted pipelines * Fix alembic divergence * Fix manual metadata logging * Add docs * Fix default names for named pipelines * Add integration tests
* [Artifacts Tab] Artifact Versioning * Fix alembic divergence * Adjust to review suggestion * Fix unit tests * Fix docstrings * Fix integration tests * Rework zenml artifact delete * Make version always string * Add integration test * Add unit test for _get_new_artifact_version * Revert "Make version always string" This reverts commit ff9a569. * Add version number field to DB & fix auto-increment versioning * Adjust integration test to include version change 10->11 * Fix unit tests * Adjust CLI messages according to review comments * Fix integration tests
* [Artifacts Tab] Rework External Artifact * Add docs * Rename artifact_name > name, artifact_version > version * Add unit tests * Rewrite external artifact integration tests * Delete outdated test * Adjust to review suggestions * Undo e2e example changes * Fix unit tests
* [Artifacts Tab] Artifact Tags & Renaming * Adjust to review suggestion * Merge migrations * Refactor ArtifactConfig and add tests * Add version to artifact update model * Delete link_output_to_model * Fix most tests * Add docs on artifact versioning and configuration * Add ArtifactConfig to public Python API * Fix circular import * Fix linking cached artifacts * OSS-2515 Rework model artifacts * Remove ArtifactConfig.overwrite_model_link * Adjust to review suggestions * Fix more integration tests * Some more integration test fixing * Fix artifact link retrieval by name
Previous PR with open discussions: #1937 |
* Move zenml.new.steps.log_artifact_metadata > zenml.artifacts.utils * WIP: redesign log_artifact_metadata * Fix unit tests * Move log_artifact_metadata integration test to tests/integration/functional/artifacts/test_utils.py * Basic save_artifact() and load_artifact() implementations * Merge zenml.utils.artifact_utils into zenml.artifacts.utils * Add integration tests and fix docstrings * Add docs on new util functions * Add ExternalArtifact to public API * Add artifact saving docs to toc * Link manually saved and loaded artifacts to step * Support zenml.load_artifact(id)
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 still have some files to review, but I am making a checkpoint here. Will continue in the morning.
docs/book/user-guide/advanced-guide/artifact-management/artifact-versioning.md
Outdated
Show resolved
Hide resolved
docs/book/user-guide/advanced-guide/artifact-management/artifact-versioning.md
Outdated
Show resolved
Hide resolved
docs/book/user-guide/advanced-guide/artifact-management/artifact-versioning.md
Outdated
Show resolved
Hide resolved
docs/book/user-guide/advanced-guide/artifact-management/artifact-versioning.md
Outdated
Show resolved
Hide resolved
docs/book/user-guide/advanced-guide/artifact-management/artifact-saving-loading.md
Outdated
Show resolved
Hide resolved
MODEL_VERSION_ARTIFACTS = "/model_version_artifacts" | ||
MODEL_VERSION_PIPELINE_RUNS = "/model_version_pipeline_runs" |
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.
@Cahllagerfeld @AlexejPenner are you aware of this and ok with it?
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'm good with most of it. My major concern: why not allowing people to use ExternalArtifact in pair with model version instead of pipeline run? I might be misunderstanding something though.
Please, also document somehow API changes and make @Cahllagerfeld aware of it before merging.
Lastly, I'm getting right that it is still possible to do Annotated[int,"my_name"]
without full-blown config object?
@@ -116,6 +117,29 @@ def get_artifact( | |||
return zen_store().get_artifact(artifact_id, hydrate=hydrate) | |||
|
|||
|
|||
@router.put( |
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.
@Cahllagerfeld @AlexejPenner please review changes below
@@ -155,15 +157,20 @@ def delete_model_version( | |||
# Model Version Artifacts | |||
########################## | |||
|
|||
model_version_artifacts_router = APIRouter( |
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.
@Cahllagerfeld @AlexejPenner please review changes below
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.
Please, take all my change requests as requests for a follow-up PR, since it is not that critical to get into another loop of aligning this PR.
@avishniakov I adjusted @Cahllagerfeld and I also just aligned on the API changes and how we will make the latest backend changes available to him in general. As for your last question, yes, |
We discussed API for artifact links with @Cahllagerfeld and the conclusion is. that we need the following fields to be in the response model (non-hydrated one, for listing even): |
@avishniakov Ok, I hydrated |
Describe changes
Backend changes required to add an "Artifacts" tab to the ZenML dashboard.
High-Level Changes
{pipeline_name}::{step_name}::output
([Artifacts Tab] Globally Unique Default Artifact Names #1867)ArtifactConfig
([Artifacts Tab] Artifact Tags, Renaming & Manual Versioning #1937)ArtifactConfig
has been heavily reworked and is now decoupled from models ([Artifacts Tab] Artifact Tags, Renaming & Manual Versioning #1937)ExternalArtifacts
have been heavily reworked and are now decoupled from models ([Artifacts Tab] Rework External Artifact #1947)ModelVersion.load_artifact()
is how artifacts of a model can now be loaded insteadModelVersionArtifact
has been heavily reworked ([Artifacts Tab] Artifact Tags, Renaming & Manual Versioning #1937)New Docs Pages
Comprehensive Code Example
This would create the following pipeline run DAGs:
Run 1:
Run 2:
Breaking Changes
ExternalArtifact
no longer supportsartifact_name
,model_name
,model_version
,model_artifact_name
, andmodel_artifact_version
argumentszenml.model.artifact_config.py
tozenml.artifacts.artifact_config.py
ArtifactConfig
no longer supportsartifact_name
andoverwrite
argsModelArtifactConfig
andDeploymentArtifactConfig
zenml.model.link_output_to_model.py
-
log_artifact_metadata()
now expects a completely different set of argumentszenml.model
, i.e.,from zenml.model import ...
no longer worksModelVersionArtifact
no longer supportsname
,link_version
,pipeline_name
, andstep_name
argumentsModelVersionPipelineRun
no longer supportsname
argumentmodel_version.get_X_artifact()
functions no longer supportpipeline_name
andstep_name
argsPre-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