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.
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
[Core] Add labels field to resources #3464
[Core] Add labels field to resources #3464
Changes from all commits
4d1df0e
f71a04b
d6df6a9
e3b01d1
6af38bd
356f4e4
5406190
36d0936
d4fda89
eba82b9
4bc4b25
9524e6c
72307b6
e7fa0a1
ebef725
2e29e87
e62eb4e
62620b3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 will cause failover unable to work. Should we just apply a global label format check instead? It might be fine to sacrifice some flexibility. Otherwise, a user have to specify the following to enable all the clouds they have which might be a bit unintuitive:
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.
Supporting failover is slightly tricky....
For example, labels in Kubernetes commonly use
/
to create a "namespace" for tags (e.g.,skypilot.co/accelerators
,app.kubernetes.io/component
.. see recommended labels). However, GCP does not support/
in labels, and a failover would cause provisioning to fail. Putting a stricter global format check would prevent users from creating these labels at all.Another option could be to not do any validation at all and let these checks fail at provisioning time. We could do that, but this seemed cleaner so went with this for now. Any other ideas? Happy to change if you think we should support failover with labels.
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 see! I think it should be fine to require the
cloud
to be presented when thelabels
is specified. Another option to support failover could be the following:Validate the labels in the following function, and return an empty list if the labels do not match the requirement for a specific cloud, i.e. make the current cloud infeasible:
skypilot/sky/clouds/cloud.py
Lines 323 to 356 in 431f567
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.
Agreed, that could work. Seems a little hacky though, since the error now becomes hidden in the warning instead of being a clear error.
Checking in resources.py (current):
Checking in get_feasible_launchable_resources with warning (last proposal):
wdyt? I'm leaning towards keeping the current variant, but can change to checking in
get_feasible_launchable_resources
to support failover if you feel strongly 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.
Keeping the current way sounds good to me, but we may want to file an issue for it for supporting failover when the labels are specified.
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.
Filed #3500!