-
Notifications
You must be signed in to change notification settings - Fork 550
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
09d791a
to
eeec764
Compare
schema_class=CuratedVisualizationSchema, | ||
session=session, | ||
) | ||
schema.update(visualization_update) |
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.
What happens if I update a curated visualization to have the same display index as another one? Does that cause issues?
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.
maybe question for @Cahllagerfeld? how are we expecting to send this from front-end
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.
Well I assume that backend should just guarantee that they have unique values? The frontend gets a list I assume, but if you want to allow users to define the order for this, it should work as expected. (Which currently, when we allow duplicate values, it doesn't. I can set something to display in position 2, but if 10 others already have the same value, it might only show up in position 11.
schema_class=CuratedVisualizationSchema, | ||
session=session, | ||
) | ||
schema.update(visualization_update) |
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.
Well I assume that backend should just guarantee that they have unique values? The frontend gets a list I assume, but if you want to allow users to define the order for this, it should work as expected. (Which currently, when we allow duplicate values, it doesn't. I can set something to display in position 2, but if 10 others already have the same value, it might only show up in position 11.
This migration introduces a new table `curated_visualization` to store visualizations associated with projects. It includes necessary columns and constraints to ensure data integrity and relationships with existing tables.
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