Artifact Version Table + Artifact Tagging #2081

fa9r · 2023-11-25T14:53:44Z

Describe changes

I split the artifact table into artifact and artifact_version so we can assign a separate set of tags to artifacts themselves.

Accordingly, I adjusted CLI, Client, models, ZenStores, and API endpoints to now have separate methods and representations for artifacts and artifact versions.

In particular, artifacts now support the following CLI commands:

zenml artifact update my_artifact -t add_this_tag -r remove_this_tag
zenml artifact version update my_artifact -v 1 -t add_this_tag -r remove_this_tag

Other changes:

zenml artifact prune now deletes all unused artifacts and artifact versions
zenml artifact delete was removed since deleting artifacts manually is too complex and error-prone to be exposed to users now

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)

src/zenml/cli/__init__.py

docs/book/getting-started/core-concepts.md

src/zenml/cli/artifact.py

src/zenml/client.py

src/zenml/models/v2/core/artifact_version.py

src/zenml/zen_stores/schemas/tag_schemas.py

src/zenml/zen_stores/zen_store_interface.py

strickvl

LGTM from my side, assuming tests pass.

schustmi

As discussed, there are a few changes we should make for RBAC to work:

Add a new ResourceType.ARTIFACT_VERSION
Update the create, update and delete endpoints for artifact versions to use this resource type
Update the read/list endpoints to use the parent artifact instead to verify whether someone can view a specific version
Update the functions get_surrogate_permission_model_for_model(...) and get_resource_type_for_model(...) in zenml.zen_server.rbac.utils.py to also handle artifact versions

src/zenml/zen_server/routers/workspaces_endpoints.py

src/zenml/zen_server/routers/artifact_version_endpoints.py

schustmi

RBAC part LGTM

bcdurak

Great changes. The concepts of Models and Artifacts seem to be much more similar now. Left a few comments, hope it helps!

bcdurak · 2023-11-29T13:36:16Z

src/zenml/zen_server/rbac/utils.py

@@ -498,6 +503,7 @@ def get_resource_type_for_model(
        SecretResponseModel: ResourceType.SECRET,
        ModelResponseModel: ResourceType.MODEL,
        ArtifactResponse: ResourceType.ARTIFACT,
+        ArtifactVersionResponse: ResourceType.ARTIFACT_VERSION,


As far as I can see, artifacts bear a lot of similarities to models in terms of their implementation. As model versions depend on models when it comes to RBAC, artifact versions also depend on artifacts.

Due to this, in the RBAC PR, model versions were left out of this mapping. Was there a different reason why we had to include them here?

@schustmi ?

src/zenml/zen_server/zen_server_api.py

bcdurak · 2023-11-29T13:59:02Z

src/zenml/models/__init__.py

    ArtifactVisualizationRequest=ArtifactVisualizationRequest,
 )
-ArtifactResponseBody.update_forward_refs(
+ArtifactVersionResponseBody.update_forward_refs(


ArtifactVersionResponseBodys also have ArtifactResponses, right? I am surprised that this is not failing due to a forward reference issue.

AFAIK a forward ref is only created if you specify your field type as a string, e.g.

class A(BaseModel): b: "OtherModel" = ...

However, in the artifact version we import the ArtifactResponse outside of type-checking and use the full type:

from zenml.models.v2.core.artifact import ArtifactResponse class ArtifactVersionResponseBody(WorkspaceScopedResponseBody): artifact: ArtifactResponse = ...

So ArtifactVersionResponseBody.artifact should not be a forward ref if I understand it right.

Maybe @schustmi can confirm?

src/zenml/models/v2/core/artifact.py

src/zenml/artifacts/unmaterialized_artifact.py

src/zenml/zen_server/routers/artifact_version_endpoints.py

bcdurak · 2023-11-29T14:16:42Z

src/zenml/model/utils.py

@@ -33,12 +33,12 @@


 def link_step_artifacts_to_model(


Should we rename this to link_step_artifact_versions_to_model as well?

We could do, no strong opinion against it

src/zenml/zen_stores/schemas/step_run_schemas.py

src/zenml/zen_stores/migrations/versions/a91762e6be36_artifact_version_table.py

bcdurak · 2023-11-29T14:30:14Z

src/zenml/zen_stores/migrations/versions/a91762e6be36_artifact_version_table.py

@@ -0,0 +1,228 @@
+"""Artifact Version Table [a91762e6be36].


I would really recommend testing this against different scenarios as it is a significant migration IMO.

Yeah agreed, so far I tested the following scenarios:

SQLite initialization from scratch

SQLite upgrade from 0.50.0 with existing artifacts

MySQL initialization from scratch

MySQL initialization from 0.50.0 with existing artifacts

Any other case I should test?

In general, I can't think of a different test.

However, In my experience, the migration failures mostly stem from (sort of) edge cases on various versions way before 0.50.0 or so. If there is such a case, (for instance where a specific field of the artifact can be possibly null), I would try to recreate this scenario and test that as well.

Artifact Version Table

ed0153b

github-actions bot added internal To filter out internal PRs and issues enhancement New feature or request labels Nov 25, 2023

fa9r added 4 commits November 25, 2023 16:22

Alembic pain

b4f0ef6

More renaming cleanup

75c1693

Fix unit tests

a600a2d

Update CLI commands

b6e43f8

fa9r marked this pull request as ready for review November 27, 2023 16:55

fa9r requested review from avishniakov, bcdurak and schustmi November 27, 2023 16:55

fa9r added 3 commits November 27, 2023 18:09

Fix integration tests except for ZenStore tests

58ee493

Fix integration tests

639eb60

Merge branch 'develop' into feature/OSS-2673-artifact-tagging

72cef48

strickvl reviewed Nov 28, 2023

View reviewed changes

Adjust to review suggestions

c40d43f

fa9r requested a review from strickvl November 29, 2023 06:46

fa9r added 5 commits November 29, 2023 09:34

Add new CLI integration tests

2cc585e

Adjust to review suggestions

ebc21f3

Merge branch 'develop' into feature/OSS-2673-artifact-tagging

8ca191a

Fix alembic order

8bc8b9a

Fix minor CLI bug

22f1947

strickvl approved these changes Nov 29, 2023

View reviewed changes

schustmi requested changes Nov 29, 2023

View reviewed changes

src/zenml/zen_server/routers/workspaces_endpoints.py Outdated Show resolved Hide resolved

fa9r added 4 commits November 29, 2023 11:36

Add new ZenStore integration tests for artifact versions

7d34954

RBAC updates

4d1a118

Fix integration tests

b95e0a7

More RBAC updates

31b3961

fa9r requested a review from schustmi November 29, 2023 12:05

schustmi requested changes Nov 29, 2023

View reviewed changes

src/zenml/zen_server/routers/artifact_version_endpoints.py Outdated Show resolved Hide resolved

fa9r added 2 commits November 29, 2023 14:18

Fix migration for existing artifacts

6796574

Fix RBAC stuff

209d7f6

fa9r requested a review from schustmi November 29, 2023 13:28

schustmi approved these changes Nov 29, 2023

View reviewed changes

bcdurak requested changes Nov 29, 2023

View reviewed changes

bcdurak mentioned this pull request Nov 29, 2023

Polymorthic run_metadata #2064

Merged

9 tasks

fa9r added 3 commits November 29, 2023 16:45

e2e example works now 🎉

103b1a6

Adjust to review suggestions

b5ce41e

Fix migration for MySQL

33073c2

fa9r requested review from bcdurak and schustmi November 29, 2023 16:53

fa9r added 2 commits November 29, 2023 18:35

Merge branch 'develop' into feature/OSS-2673-artifact-tagging

bbfc37f

Fix migration for SQLite again...

1ed3766

bcdurak approved these changes Nov 30, 2023

View reviewed changes

fa9r merged commit ac0edbb into develop Nov 30, 2023
30 checks passed

fa9r deleted the feature/OSS-2673-artifact-tagging branch November 30, 2023 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Artifact Version Table + Artifact Tagging #2081

Artifact Version Table + Artifact Tagging #2081

fa9r commented Nov 25, 2023 •

edited

Loading

strickvl left a comment

schustmi left a comment

schustmi left a comment

bcdurak left a comment

bcdurak Nov 29, 2023

fa9r Nov 29, 2023

bcdurak Nov 29, 2023

fa9r Nov 29, 2023 •

edited

Loading

bcdurak Nov 29, 2023

fa9r Nov 29, 2023

bcdurak Nov 29, 2023

fa9r Nov 29, 2023

bcdurak Nov 29, 2023

Artifact Version Table + Artifact Tagging #2081

Artifact Version Table + Artifact Tagging #2081

Conversation

fa9r commented Nov 25, 2023 • edited Loading

Describe changes

Pre-requisites

Types of changes

strickvl left a comment

Choose a reason for hiding this comment

schustmi left a comment

Choose a reason for hiding this comment

schustmi left a comment

Choose a reason for hiding this comment

bcdurak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fa9r Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fa9r commented Nov 25, 2023 •

edited

Loading

fa9r Nov 29, 2023 •

edited

Loading