Skip to content
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

GCP/Azure: add skypilot-user tags to VMs on these clouds. #1593

Merged
merged 4 commits into from
Jan 16, 2023

Conversation

concretevitamin
Copy link
Collaborator

For better accountability.

Tested (run the relevant ones):

GCP head + worker (note: they have "labels" and "tags"; former is used):

Screen Shot 2023-01-14 at 07 48 25

Azure head + worker:

Screen Shot 2023-01-14 at 07 52 49

Screen Shot 2023-01-14 at 07 52 57

Also tested (previously was throwing an error from within optimizer, No resource satisfying None on Azure):

» sky launch --use-spot -i10 --down --cloud azure
Usage: sky launch [OPTIONS] [ENTRYPOINT]...
Try 'sky launch -h' for help.

Error: SkyPilot currently has not implemented support for spot instances on Azure. Please file an issue if you need this feature.

Copy link
Collaborator

@WoosukKwon WoosukKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I actually experienced the difficulties in identifying the owner of the VMs in our GCP and Azure accounts. The new update would be really helpful. Thanks for adding this @concretevitamin!

# A basic check. Programmatic calls will have a proper (but less
# informative) error from optimizer.
if (cloud is not None and cloud.lower() == 'azure' and
use_spot is not None and use_spot):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't it be shortened as use_spot?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. This is religiously following the styleguide where we explicitly test None rather than implicitly.

@concretevitamin
Copy link
Collaborator Author

Thanks @WoosukKwon. Merging this.

@concretevitamin concretevitamin merged commit fe1b3ab into master Jan 16, 2023
@concretevitamin concretevitamin deleted the gcp-tags branch January 16, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants