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] honor allowed_cloud config in catalog lookup #4496

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aylei
Copy link

@aylei aylei commented Dec 20, 2024

follow up #4483 (comment)

This PR will speed up sky launch when the number of allowed clouds is small and a non-canonical GPU name is requested. As a test, with only runpod allowed in both case, this PR speed up sky launch by 4x:

# PR branch
$ /usr/bin/time sky launch --gpus L40:1
Considered resources (1 node):
---------------------------------------------------------------------------------------------
 CLOUD    INSTANCE        vCPUs   Mem(GB)   ACCELERATORS   REGION/ZONE   COST ($)   CHOSEN
---------------------------------------------------------------------------------------------
 RunPod   1x_L40_SECURE   16      48        L40:1          CA            1.14          ✔
---------------------------------------------------------------------------------------------
        0,76 real         0,89 user         2,72 sys

# master branch
$ /usr/bin/time sky launch --gpus L40:1
Considered resources (1 node):
---------------------------------------------------------------------------------------------
 CLOUD    INSTANCE        vCPUs   Mem(GB)   ACCELERATORS   REGION/ZONE   COST ($)   CHOSEN
---------------------------------------------------------------------------------------------
 RunPod   1x_L40_SECURE   16      48        L40:1          CA            1.14          ✔
---------------------------------------------------------------------------------------------
        2,94 real         1,62 user         2,61 sys

@Michaelvll PTAL, thanks!

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (see test above)
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

@aylei
Copy link
Author

aylei commented Dec 20, 2024

/quicktest-core

@concretevitamin
Copy link
Member

Nice @aylei! QQ: Some users may not explicitly set allowed_clouds and they may have very few clouds (e.g., 1) actually enabled. Namely, they may have run sky check before and we noted down 1 enabled cloud. Does this PR speedup those cases as well?

@aylei
Copy link
Author

aylei commented Dec 21, 2024

Nice @aylei! QQ: Some users may not explicitly set allowed_clouds and they may have very few clouds (e.g., 1) actually enabled. Namely, they may have run sky check before and we noted down 1 enabled cloud. Does this PR speedup those cases as well?

Good catch! I will update the PR to honor enabled clouds instead.

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