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.

Sign up for GitHub

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

Jump to bottom

[Artifacts Tab] Globally Unique Default Artifact Names #1867

Merged
merged 9 commits into from
Oct 13, 2023
Merged

[Artifacts Tab] Globally Unique Default Artifact Names #1867

merged 9 commits into from
Oct 13, 2023

Conversation

fa9r
Copy link
Contributor

@fa9r fa9r commented Oct 9, 2023 •
edited
Loading

Describe changes

Artifacts now have globally unique default names {pipeline_name}::{step_name}::output.

Note that this only affects the name of the artifact itself (artifact.name), not the respective output name (step.outputs[...]). This means, e.g., that step.outputs["output"] will still work as before, while step.outputs["output"].name now returns {pipeline_name}::{step_name}::output instead of output.

I also added a has_custom_name field into the artifact DB table, which we can use in the frontend to decide which artifacts to show by default in the artifact list (the idea is that only named ones are shown by default to prevent clutter)

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)

@strickvl strickvl added enhancement New feature or request internal To filter out internal PRs and issues labels Oct 9, 2023
avishniakov
avishniakov reviewed Oct 9, 2023
View reviewed changes
Copy link
Contributor

@avishniakov avishniakov left a comment

Choose a reason for hiding this comment

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

Left a few conceptual comments, not critical

src/zenml/orchestrators/step_runner.py Show resolved Hide resolved
src/zenml/models/artifact_models.py Show resolved Hide resolved
@fa9r fa9r requested a review from avishniakov October 12, 2023 09:24
strickvl
strickvl approved these changes Oct 12, 2023
View reviewed changes
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.

LGTM! Had looked at the first half previously, but finished the rest now and seems fine!

@fa9r fa9r merged commit f41aa6f into feature/OSS-2190-data-as-first-class-citizen Oct 13, 2023
27 of 29 checks passed
@fa9r fa9r deleted the feature/OSS-2497-unique-default-output-names branch October 13, 2023 07:51
@fa9r fa9r mentioned this pull request Nov 17, 2023
Merged
9 tasks
fa9r added a commit that referenced this pull request Nov 23, 2023
@fa9r
Artifacts Tab (#1943)
845adf8 
* [Artifacts Tab] Globally Unique Default Artifact Names (#1867)

* [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 (#1869)

* [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

* Fix alembic order

* [Artifacts Tab] Rework External Artifact (#1947)

* [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 & Manual Versioning (#1937)

* [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

* [Artifacts Tab] Artifact Util Functions (#2038)

* 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)

* Restore deleted integration tests that are potentially still relevant

* Add ModelVersion.load_artifact() util function

* Adjust artifact tagging to use new tags table

* Silence overlapping tags column warning

* Fix ZenStore integration tests

* Refactor model CLI methods and adjust Client to return response models again

* Fix artifact config linking

* Refactor model version links list API / client / zen store interfaces

* Fix external artifact run linking and remaining integration tests

* Adjust to review suggestions

* Add missing routers

* Add workspace-scoped create endpoints back in

* Undo uninteded workspace endpoint changes

* Remove ExternalArtifact pipeline_run and pipeline args

* Adjust CLI commands to review suggestions

* Update e2e example reference

* Prevent duplicate artifact versions

* Hydrate artifacts/runs into the link responses

* Fix integration tests and docstrings

* Relax URI checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@strickvl strickvl strickvl approved these changes

@avishniakov avishniakov avishniakov approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request internal To filter out internal PRs and issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fa9r @strickvl @avishniakov