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

Polymorthic run_metadata #2064

Merged
merged 20 commits into from
Dec 4, 2023
Merged

Polymorthic run_metadata #2064

merged 20 commits into from
Dec 4, 2023

Conversation

avishniakov
Copy link
Contributor

@avishniakov avishniakov commented Nov 20, 2023 •
edited
Loading

Describe changes

I reworked run_metadata table to accept resource_id and resoruce_type as identifiers going forward, instead of growing number of columns we now add a new resource type and it would be done.

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)

@github-actions github-actions bot added internal To filter out internal PRs and issues enhancement New feature or request labels Nov 20, 2023
fa9r
fa9r approved these changes Nov 21, 2023
View reviewed changes
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.

Nice, I left a few nitpicks but overall this looks good to merge already 🚀

src/zenml/zen_server/routers/run_metadata_endpoints.py Show resolved Hide resolved
src/zenml/enums.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/migrations/versions/8ed03137cacc_polymorthic_run_metadata.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/migrations/versions/8ed03137cacc_polymorthic_run_metadata.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/migrations/versions/8ed03137cacc_polymorthic_run_metadata.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/migrations/versions/8ed03137cacc_polymorthic_run_metadata.py Outdated Show resolved Hide resolved
bcdurak
bcdurak requested changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@bcdurak bcdurak left a comment

Choose a reason for hiding this comment

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

Great changes. It looks much cleaner now. Unfortunately, it has a few clashes with the PR here. As I mentioned in my notes, we have to coordinate our efforts here.

src/zenml/enums.py Outdated Show resolved Hide resolved
src/zenml/models/v2/core/run_metadata.py Outdated Show resolved Hide resolved
src/zenml/models/v2/core/run_metadata.py
)
step_run_id: Optional[UUID] = Field(
title="The ID of the step run that this metadata belongs to."
resource_id: UUID = Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to put the resource_id and the resource_type into the body of the response as they are essential to a RunMetadataResponse. WDYT?

Copy link
Contributor

@fa9r fa9r Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree, in almost all cases you will access these models through the parent entity, e.g., artifact.run_metadata / ... in which case you already know what the resource type and ID are

src/zenml/zen_server/routers/workspaces_endpoints.py Outdated
step = zen_store().get_run_step(run_metadata.resource_id)
verify_permission_for_model(step, action=Action.UPDATE)
elif run_metadata == MetadataResourceTypes.ARTIFACT:
artifact = zen_store().get_artifact(run_metadata.resource_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I am leaving notes where the artifact version PR might affect the current implementation. You might have to get the artifact_version first and then check for the artifact within that response.

Copy link
Contributor

Choose a reason for hiding this comment

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

artifact versions have their own RBAC entity so I don't think you need that

src/zenml/zen_stores/schemas/artifact_schemas.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/schemas/run_metadata_schemas.py Outdated Show resolved Hide resolved
src/zenml/artifacts/utils.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/migrations/versions/8ed03137cacc_polymorthic_run_metadata.py Show resolved Hide resolved
@fa9r
Copy link
Contributor

fa9r commented Nov 30, 2023

@avishniakov @bcdurak since my PR caused all the conflicts and since I already reviewed this one, I already went ahead and resolved the merge conflicts between them. While at it, I also addressed the minor open reviewer requests. IMO this should be ready to merge now, @bcdurak WDYT?

@fa9r fa9r requested a review from bcdurak November 30, 2023 11:37
@avishniakov avishniakov merged commit 6530bb2 into develop Dec 4, 2023
32 checks passed
@avishniakov avishniakov deleted the feature/OSS-2574-polymorthic-run-metadata branch December 4, 2023 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fa9r fa9r fa9r approved these changes

@bcdurak bcdurak bcdurak 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
@avishniakov @fa9r @bcdurak