-
Notifications
You must be signed in to change notification settings - Fork 534
Run template rework #3856
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?
Run template rework #3856
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
reviewed 28/38 files so far - my comments at this point:
"""The `runnable` property. | ||
|
||
Returns: | ||
the value of the property. |
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.
I feel we could elaborate more here, if I know nothing and try to inspect what it means to be runnable, I would love to actually be told somewhere.
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.
HAHA just resolved it uncommented, sneaky - I still think this docstring is useless, just added to pass linting - what does it mean to be runnable? If I am in my IDE and i do pipeline_deploymnet.runnable - i would love to be told what it means to be runnable
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.
Yes this is just for linting, same as the 1000 other model properties where we do the same thing. This is better than having to manage the same explanation twice, and that can be found in the pedantic model field that this property references.
ZenML CLI Performance Comparison (Threshold: 1.0s, Timeout: 60s, Slow: 5s)❌ Failed Commands on Current Branch (feature/run-template-improvements)
🚨 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.
Reviewed everything besides client.py and sql_zen_store - will return for those
"""The `runnable` property. | ||
|
||
Returns: | ||
the value of the property. |
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.
HAHA just resolved it uncommented, sneaky - I still think this docstring is useless, just added to pass linting - what does it mean to be runnable? If I am in my IDE and i do pipeline_deploymnet.runnable - i would love to be told what it means to be runnable
@@ -223,6 +240,7 @@ def get_deployment( | |||
get_method=zen_store().get_deployment, | |||
hydrate=hydrate, | |||
step_configuration_filter=step_configuration_filter, | |||
include_config_schema=include_config_schema, |
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.
is this for simplifying the frontend?
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.
Not so much about simplifying, but including this schema is quite expensive and not necessary anywhere unless trying to run the deployment. The deployment itself gets fetched in quite a few places, which is why I added this.
raise ValueError( | ||
"Unable to create deployment versions as a component of the " | ||
"associated stack has a custom flavor." | ||
) |
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.
Is there any plan to deal with this in the future, even with a workaround?
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.
Workaround - not possible I think.
Idea how to solve it exists, but I don't think the implementation will happen anytime soon
Describe changes
This PR prepares the removal of run templates, and instead exposes
PipelineDeployments
as a user-facing concept as a replacement.Run templates are already a very thin wrapper (name, description, tags) around the underlying pipeline deployment. Instead of having this wrapper entity, this PR exposes the deployment itself as something that can be triggered from the server and viewed in the dashboard. To achieve feature parity with run templates, these deployments can now have version names and tags. One difference to run templates is that these deployments will require a unique version name per pipeline, instead of the globally unique name that run templates require.
TODO:
ZENML_SERVER_MAX_CONCURRENT_TEMPLATE_RUNS
env variable toZENML_SERVER_MAX_CONCURRENT_DEPLOYMENT_RUNS
Implementation decisions:
Frontend changes:
triggered_run
, which cause the current implementation to fail. We should instead display them and link to the triggered pipeline run.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