-
Notifications
You must be signed in to change notification settings - Fork 446
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
Allow skypilot to configure step or run full pipeline in one VM #2276
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe recent updates focus on enhancing the SkyPilot VM Orchestrator within the ZenML framework. The modifications allow for more granular control over resource allocation, enabling users to specify VM types and resources for individual pipeline steps or to run the entire pipeline on a single VM, depending on the new settings introduced. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Left one structural feedbacl
src/zenml/integrations/skypilot/orchestrators/skypilot_base_vm_orchestrator.py
Show resolved
Hide resolved
@coderabbitai review |
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- docs/book/stacks-and-components/component-guide/orchestrators/skypilot-vm.md (2 hunks)
- src/zenml/integrations/skypilot/flavors/skypilot_orchestrator_base_vm_config.py (1 hunks)
- src/zenml/integrations/skypilot/orchestrators/skypilot_base_vm_orchestrator.py (3 hunks)
Files skipped from review due to trivial changes (1)
- docs/book/stacks-and-components/component-guide/orchestrators/skypilot-vm.md
Additional comments: 3
src/zenml/integrations/skypilot/flavors/skypilot_orchestrator_base_vm_config.py (1)
- 116-125: The addition of the
disable_step_based_settings
attribute with a default value ofFalse
is consistent with the PR's objective to allow configuration of step or run full pipeline in one VM. The docstring is clear and explains the purpose of the attribute well.src/zenml/integrations/skypilot/orchestrators/skypilot_base_vm_orchestrator.py (2)
114-121: The new
config
property provides a typed way to access the orchestrator configuration. This is a good practice as it ensures type safety and clarity in the code.203-235: The logic to run the entire pipeline in one VM or separate VMs based on the
disable_step_based_settings
configuration is implemented correctly. However, ensure that the rest of the codebase and any external documentation are updated to reflect this new behavior, as it could potentially affect existing users' workflows.Verification successful
The search within Python files for the
disable_step_based_settings
property has yielded results, indicating that this property is indeed present and used within the codebase. Specifically, it is referenced in theskypilot_orchestrator_base_vm_config.py
file, where it is defined with a default value ofFalse
, and in theskypilot_base_vm_orchestrator.py
file, where it is used within a conditional statement.Additionally, the documentation has been updated to reflect the new behavior, as the search in Markdown files within the
docs
directory has found references todisable_step_based_settings
.Based on the information gathered, it can be concluded that both the codebase and the documentation have been updated to accommodate the new
disable_step_based_settings
configuration.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the documentation is updated to reflect the new behavior. rg --type md 'disable_step_based_settings' ./docs # Verify that the rest of the codebase is aware of the new config property. ast-grep --lang python --pattern $'SkypilotBaseOrchestratorConfig { $$$ disable_step_based_settings $$$ }'Length of output: 1253
Script:
#!/bin/bash # Correcting the ast-grep pattern to search for the 'disable_step_based_settings' attribute within the 'SkypilotBaseOrchestratorConfig' class. ast-grep --lang python --pattern $'class SkypilotBaseOrchestratorConfig { $$$ disable_step_based_settings $$$ }'Length of output: 464
Script:
#!/bin/bash # Attempting a broader search for 'disable_step_based_settings' within Python files to verify its usage in the codebase. rg --type py 'disable_step_based_settings'Length of output: 617
src/zenml/integrations/skypilot/orchestrators/skypilot_base_vm_orchestrator.py
Outdated
Show resolved
Hide resolved
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.
Approved pending that final comment I just made. Otherwise looks good!
…_orchestrator.py Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com>
…:zenml-io/zenml into feature/skypilot-one-vm-run-orchestrator
@coderabbitai review |
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- src/zenml/integrations/skypilot/orchestrators/skypilot_base_vm_orchestrator.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/zenml/integrations/skypilot/orchestrators/skypilot_base_vm_orchestrator.py
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.
Nice one!
docs/book/stacks-and-components/component-guide/orchestrators/skypilot-vm.md
Outdated
Show resolved
Hide resolved
…skypilot-vm.md Co-authored-by: Hamza Tahir <hamza@zenml.io>
…l-io#2276) * Allow skypilot to configure step or run full pipeline in one VM * Chnage behaviour of using step based resources * Update src/zenml/integrations/skypilot/orchestrators/skypilot_base_vm_orchestrator.py Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> * Refactor SkypilotBaseOrchestrator to support step-based VM configuration * Update docs/book/stacks-and-components/component-guide/orchestrators/skypilot-vm.md Co-authored-by: Hamza Tahir <hamza@zenml.io> --------- Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> Co-authored-by: Hamza Tahir <hamza@zenml.io>
Describe changes
I implemented/fixed _ to achieve _.
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
Summary by CodeRabbit
New Features
Documentation
Enhancements