-
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
Changes from 36 commits
27fc323
6f50333
f109795
11a18b9
db2c007
4471d48
57ed6df
0e02dee
7452667
32de4aa
0eb1f97
4270edd
dfdedf9
b6c9f4c
301bfa6
f9d51d8
559b710
7476f85
09c368d
4077eda
5057bae
b20b975
8699b5a
9898aef
ec7d09b
21af350
69b5924
9964220
f65e61d
b33d4bb
daaaefc
dddd6fe
a3271a6
794cba4
f40796f
46170b6
89903b8
44ea432
df16014
69a7ce4
7ef6179
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,52 +30,52 @@ | |
SKYPILOT_GCP_ORCHESTRATOR_FLAVOR = "vm_gcp" | ||
SKYPILOT_AZURE_ORCHESTRATOR_FLAVOR = "vm_azure" | ||
|
||
class SkypilotGCPIntegration(Integration): | ||
"""Definition of Skypilot Integration for ZenML.""" | ||
|
||
class SkypilotAWSIntegration(Integration): | ||
"""Definition of Skypilot AWS Integration for ZenML.""" | ||
|
||
NAME = SKYPILOT_AWS | ||
REQUIREMENTS = ["skypilot[aws]"] | ||
NAME = SKYPILOT_GCP | ||
REQUIREMENTS = ["skypilot[aws,gcp,azure]<=0.4.1"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need all the 3 clouds for one integration? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a potential bug that we have a ticket for to investigate, it doesn't matter what cloud we use, zenml always pick up skypilot[aws] and install it in the docker image. So the workaround was to set all 3 of them. |
||
APT_PACKAGES = ["openssh-client","rsync"] | ||
|
||
@classmethod | ||
def flavors(cls) -> List[Type[Flavor]]: | ||
"""Declare the stack component flavors for the Skypilot AWS integration. | ||
"""Declare the stack component flavors for the Skypilot GCP integration. | ||
|
||
Returns: | ||
List of stack component flavors for this integration. | ||
""" | ||
from zenml.integrations.skypilot.flavors import ( | ||
SkypilotAWSOrchestratorFlavor, | ||
SkypilotGCPOrchestratorFlavor, | ||
) | ||
|
||
return [SkypilotAWSOrchestratorFlavor] | ||
|
||
return [SkypilotGCPOrchestratorFlavor] | ||
|
||
class SkypilotGCPIntegration(Integration): | ||
"""Definition of Skypilot Integration for ZenML.""" | ||
class SkypilotAWSIntegration(Integration): | ||
"""Definition of Skypilot AWS Integration for ZenML.""" | ||
|
||
NAME = SKYPILOT_GCP | ||
REQUIREMENTS = ["skypilot[gcp]"] | ||
NAME = SKYPILOT_AWS | ||
REQUIREMENTS = ["skypilot[aws,gcp,azure]<=0.4.1"] | ||
APT_PACKAGES = ["openssh-client","rsync"] | ||
safoinme marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
@classmethod | ||
def flavors(cls) -> List[Type[Flavor]]: | ||
"""Declare the stack component flavors for the Skypilot GCP integration. | ||
"""Declare the stack component flavors for the Skypilot AWS integration. | ||
|
||
Returns: | ||
List of stack component flavors for this integration. | ||
""" | ||
from zenml.integrations.skypilot.flavors import ( | ||
SkypilotGCPOrchestratorFlavor, | ||
SkypilotAWSOrchestratorFlavor, | ||
) | ||
|
||
return [SkypilotGCPOrchestratorFlavor] | ||
|
||
return [SkypilotAWSOrchestratorFlavor] | ||
|
||
class SkypilotAzureIntegration(Integration): | ||
"""Definition of Skypilot Integration for ZenML.""" | ||
|
||
NAME = SKYPILOT_AZURE | ||
REQUIREMENTS = ["skypilot[azure]"] | ||
REQUIREMENTS = ["skypilot[aws,gcp,azure]<=0.4.1"] | ||
APT_PACKAGES = ["openssh-client","rsync"] | ||
|
||
@classmethod | ||
def flavors(cls) -> List[Type[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.
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