-
Notifications
You must be signed in to change notification settings - Fork 548
Deployment custom visualizations #4016
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
base: develop
Are you sure you want to change the base?
Conversation
@safoinme Can you write a description for this PR please, and include why we need all of this? |
This change sets the `include_metadata` and `include_resources` parameters to `False` in the `visualization.to_model` method call within the `DeploymentSchema` class. This adjustment ensures that unnecessary metadata and resources are not included in the visualizations. No functional changes are expected as a result of this update.
ZenML CLI Performance Comparison (Threshold: 1.0s, Timeout: 60s, Slow: 5s)❌ Failed Commands on Current Branch (feature/deployment-custom-visualizations)
🚨 New Failures IntroducedThe following commands fail on your branch but worked on the target branch:
Performance Comparison
Summary
Environment Info
|
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.
This feature has the potential of becoming more than just a deployment association. In fact, I would even say that attaching visualizations to deployments is the least useful of the possible range of associations that can happen at pipeline, pipeline run and pipeline snapshot level.
If it's not too late, I'll still recommend that this be implemented using a more generic and extensible approach, similar to how run metadata is implemented (see RunMetadataResourceSchema
).
Documentation Link Check Results❌ Absolute links check failed |
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.
This looks mostly great and I love how extensible you've made it. That being said, I do have a couple of high-level comments that will require you to change things a bit:
- the CuratedVisualization models and Client methods don't reflect the UX expectations (see my comments)
- you still need to add support in the schema for all the various resources
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.
The RBAC permission checks look good now, but I discovered another problem with how the SQL schemas are implemented: they are over-engineered and the foreign relationships still need some work.
src/zenml/zen_server/routers/curated_visualization_endpoints.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Stefan Nica <[email protected]>
Co-authored-by: Stefan Nica <[email protected]>
Co-authored-by: Stefan Nica <[email protected]>
Co-authored-by: Stefan Nica <[email protected]>
Co-authored-by: Stefan Nica <[email protected]>
…thub.com/zenml-io/zenml into feature/deployment-custom-visualizations
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.
The last round.
src/zenml/zen_server/routers/curated_visualization_endpoints.py
Outdated
Show resolved
Hide resolved
src/zenml/zen_server/routers/curated_visualization_endpoints.py
Outdated
Show resolved
Hide resolved
src/zenml/zen_server/routers/curated_visualization_endpoints.py
Outdated
Show resolved
Hide resolved
src/zenml/zen_stores/migrations/versions/0ede689b266a_add_vizualisations_and_link_them_to_.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Stefan Nica <[email protected]>
Co-authored-by: Stefan Nica <[email protected]>
Co-authored-by: Stefan Nica <[email protected]>
Describe changes
Why
This is related to #4007 and the Custom visualisation ticket in Notion
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