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

[core] Add allowed_clouds to config to check only specific clouds #3556

Merged
merged 8 commits into from
May 17, 2024

Conversation

romilbhardwaj
Copy link
Collaborator

@romilbhardwaj romilbhardwaj commented May 15, 2024

Adds a allowed_clouds field to config.yaml which limits the clouds that will be checked/enabled by sky check.

allowed_clouds:
  - kubernetes
  - aws
  - gcp

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Manual tests:
    • With allowed_clouds not specified - sky check.
    • With allowed_clouds: [aws, cloudflare] - sky check, sky check aws, sky check gcp. Note is shown when checking sky check and sky check gcp.
    • With allowed_clouds: [aws, gcp] with other clouds enabled already - sky check. Other clouds will be disabled.

@romilbhardwaj romilbhardwaj marked this pull request as ready for review May 17, 2024 03:18
@romilbhardwaj
Copy link
Collaborator Author

Ready for review!

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks for adding the support for this @romilbhardwaj! Looks mostly good to me. Should we update the PR title as well?

sky/check.py Outdated Show resolved Hide resolved
sky/check.py Outdated Show resolved Hide resolved
sky/utils/schemas.py Show resolved Hide resolved
@romilbhardwaj romilbhardwaj changed the title [wip][core] Add candidate_clouds to config to check only specific clouds [wip][core] Add allowed_clouds to config to check only specific clouds May 17, 2024
@romilbhardwaj romilbhardwaj changed the title [wip][core] Add allowed_clouds to config to check only specific clouds [core] Add allowed_clouds to config to check only specific clouds May 17, 2024
@romilbhardwaj
Copy link
Collaborator Author

Thanks @Michaelvll! Ready for another look.

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks for adding this support @romilbhardwaj! LGTM.

skipped_clouds_hint = None
if skipped_clouds:
skipped_clouds_hint = (
'\nNote: The following clouds were disabled because they were not '
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we now changed the wording to disabled, we may want to always show the hint even if sky check gcp is used, as we will show the enabled clouds with the old enabled clouds included as well. : )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm do you mean even when allowed_clouds is not specified in config.yaml, sky check <cloud> should always show the hint above?

I think in that case hint should not be shown because those clouds were not disabled because they were not in allowed_list, but rather because of other errors while checking (which are surfaced as <cloud>: disabled along with reason). wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, I mean when the allowed_clouds is specified and sky check <cloud> is used. It seems there is no hint showing in that case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a ~/.sky/config.yaml, and when I do sky check gcp the following shows up, without the hint:

allowed_clouds: ['aws', 'gcp', 'cloudflare']
  GCP: enabled                              

To enable a cloud, follow the hints above and rerun: sky check 
If any problems remain, refer to detailed docs at: https://skypilot.readthedocs.io/en/latest/getting-started/installation.html

🎉 Enabled clouds 🎉
  ✔ AWS
  ✔ GCP

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a nit which should not block us merging the PR :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh got it! It should be fixed now :)

@romilbhardwaj romilbhardwaj merged commit 6968be5 into master May 17, 2024
20 checks passed
@romilbhardwaj romilbhardwaj deleted the core_remotes branch May 17, 2024 08:10
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.

2 participants