-
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 multi step configuration for skypilot #2166
Merged
safoinme
merged 41 commits into
develop
from
feature/OSS-2680-multi-image-multi-vm-skypilot
Jan 12, 2024
Merged
Changes from 2 commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
27fc323
allow multi step configuration for skypilot
safoinme 6f50333
fixes
safoinme f109795
Merge branch 'develop' into feature/OSS-2680-multi-image-multi-vm-sky…
strickvl 11a18b9
Merge branch 'develop' into feature/OSS-2680-multi-image-multi-vm-sky…
safoinme db2c007
Merge branch 'feature/OSS-2680-multi-image-multi-vm-skypilot' of gith…
safoinme 4471d48
Auto-update of Starter template
actions-user 57ed6df
update skypilot to allow running using an orchestrating VM
safoinme 0e02dee
Merge branch 'feature/OSS-2680-multi-image-multi-vm-skypilot' of gith…
safoinme 7452667
add ability to create an aws config when configuring local client
safoinme 32de4aa
Add advanced AWS configurations to config.yaml
safoinme 0eb1f97
Fix cluster name concatenation in skypilot orchestrator entrypoint
safoinme 4270edd
Fix AWS service connector and Skypilot orchestrator issues
safoinme dfdedf9
Add APT packages to Skypilot integrations and sanitize cluster name
safoinme b6c9f4c
Remove advanced AWS configurations from config.yaml
safoinme 301bfa6
Update Skypilot orchestrator code
safoinme f9d51d8
Merge branch 'develop' into feature/OSS-2680-multi-image-multi-vm-sky…
safoinme 559b710
Auto-update of Starter template
actions-user 7476f85
Refactor cluster name sanitization in SkypilotBaseOrchestrator
safoinme 09c368d
merge changes
safoinme 4077eda
Refactor SkypilotBaseOrchestrator and SkypilotOrchestratorEntrypoint
safoinme 5057bae
Remove commented out code and update Skypilot orchestrator entrypoint
safoinme b20b975
Fix GCP service connector support for local gcloud CLI login
stefannica 8699b5a
Fix Skypilot orchestrator cluster name generation
safoinme 9898aef
Merge branch 'feature/OSS-2680-multi-image-multi-vm-skypilot' of gith…
safoinme ec7d09b
Merge branch 'develop' into feature/OSS-2680-multi-image-multi-vm-sky…
safoinme 21af350
Refactor cluster creation logic in SkypilotBaseOrchestrator
safoinme 69b5924
Allow custom docker run args when using the Skypilot orchestrator
schustmi 9964220
Handle exception and log error message in SkypilotBaseOrchestrator
safoinme f65e61d
Refactor AWS service connector to improve file handling
safoinme b33d4bb
Merge branch 'feature/OSS-2680-multi-image-multi-vm-skypilot' of gith…
safoinme daaaefc
Refactor Skypilot integrations for ZenML
safoinme dddd6fe
Merge branch 'develop' into feature/OSS-2680-multi-image-multi-vm-sky…
safoinme a3271a6
Add encoding parameter to subprocess call
safoinme 794cba4
Update Skypilot integration for AWS, GCP, and Azure
safoinme f40796f
Add note about configuring pipeline resources for specific orchestrat…
safoinme 46170b6
Merge branch 'develop' into feature/OSS-2680-multi-image-multi-vm-sky…
strickvl 89903b8
Update src/zenml/integrations/gcp/service_connectors/gcp_service_conn…
safoinme 44ea432
Merge branch 'develop' into feature/OSS-2680-multi-image-multi-vm-sky…
safoinme df16014
Add error handling for running code in a notebook and update Skypilot…
safoinme 69a7ce4
Merge branch 'feature/OSS-2680-multi-image-multi-vm-skypilot' of gith…
safoinme 7ef6179
Merge branch 'develop' into feature/OSS-2680-multi-image-multi-vm-sky…
safoinme File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
shouldnt we take this from
ResourceSettings
actually now that I think about it?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 and the GPU
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 think it would be better to distinguish between both of them, because
ResourceSettings
can be defined for things like pods which would allow random resource given, while for Skypilot most of these parameters must match if supplied together because a VM type with this specific resource must existThere 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.
IMO the
cpu_count
andmemory
in ResourceSettings should be mapped here... what is the point of ResourceSettings otherwise? @schustmi would love your feedback