-
Notifications
You must be signed in to change notification settings - Fork 551
Dynamic pipelines v0 #4074
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?
Dynamic pipelines v0 #4074
Conversation
4c02511 to
c6153aa
Compare
08b3069 to
250e5d3
Compare
250e5d3 to
e217f58
Compare
575843d to
226b65f
Compare
597b9ce to
a83c54c
Compare
e82964c to
093ee9a
Compare
093ee9a to
b0b6162
Compare
cffc807 to
ec7bae4
Compare
b13c693 to
70bf95b
Compare
8c13c97 to
8162fd4
Compare
8162fd4 to
1bb1cd3
Compare
ZenML CLI Performance Comparison (Threshold: 1.0s, Timeout: 60s, Slow: 5s)❌ Failed Commands on Current Branch (feature/dynamic-pipelines)
🚨 New Failures IntroducedThe following commands fail on your branch but worked on the target branch:
Performance Comparison
Summary
Environment Info
|
d5f90e8 to
e902eaf
Compare
| """ | ||
| return self._wrapped.result() | ||
|
|
||
| def load(self) -> Any: |
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.
Perhaps we should keep a cache dictionary for materialized artifacts, so if (for some weird reason) users do multiple load calls (instead of assigning and reusing) we can return the cached results.
| else: | ||
| raise ValueError(f"Invalid step run output: {result}") | ||
|
|
||
| def __getitem__(self, key: Union[str, int]) -> ArtifactFuture: |
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 we should provide a custom implementation for setitem as a better UX/guidance to users than getting an error like this: object does not support item assignment.
| """ | ||
| if isinstance(key, str): | ||
| index = self._output_keys.index(key) | ||
| elif isinstance(key, int): |
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 would recommend adopting one kind of wrapper behavior and stick to it. If we enable users to access futures both dict-style and list/tuple style I think the more we extend the class magic functions the more the behavior may be more confusing regarding what gets executed and what the users expects to be executed.
Plus in general accessing by key is a safer operation and would be a good pattern to enforce. The results may change length, order and accessing by position is a bit un-safe.
| index=index, | ||
| ) | ||
|
|
||
| def __iter__(self) -> Any: |
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.
Would __contains__ make sense to also implement here? 🤔
Describe changes
This PR implements dynamic pipelines for the local/kubernetes orchestrators.
Example
Features
in_process=Falsefor a step, the orchestrator will launch a separate container in which the step will be executed (only works if using the kubernetes orchestrator):step.submit(...). This will either execute the step or launch a new container in a new thread.depends_on:Known issues
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes