-
Notifications
You must be signed in to change notification settings - Fork 640
[Core] Allow nonexistent cloud in candidate resources and speed up optimization #3567
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] Allow nonexistent cloud in candidate resources and speed up optimization #3567
Conversation
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.
Thanks @Michaelvll for the fix! It looks mostly good to me. Left several nits and a discussion on multiple sky.check
in a dag pipeline ; )
sky/optimizer.py
Outdated
is_or_are = 'is' if len(rechecked_but_disabled_clouds) == 1 else 'are' | ||
task_name = f' {task.name!r}' if task.name is not None else '' | ||
msg = ( | ||
f'Task{task_name} requires ' |
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.
f'Task{task_name} requires ' | |
f'Task{task_name} requires one of ' |
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.
It is not actually one of
but requested all of them : )
Co-authored-by: Tian Xia <[email protected]>
…om:skypilot-org/skypilot into allow-unexist-cloud-in-candidate-resources
Thanks for the review @cblmemo! PTAL @romilbhardwaj @cblmemo. |
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.
Thanks @Michaelvll! Works nicely!
@@ -120,6 +120,8 @@ def optimize(dag: 'dag_lib.Dag', | |||
for a task. | |||
exceptions.NoCloudAccessError: if no public clouds are enabled. | |||
""" | |||
_check_specified_clouds(dag) |
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.
nit - Can we add a quick comment why we call this method and the fact that it will run sky check
on the clouds specified in resources?
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.
Or alternatively, docstr for _check_specified_clouds
would be useful
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.
Added a docstr for the function. Thanks!
enabled_clouds = sky_check.get_cached_enabled_clouds_or_refresh( | ||
raise_if_no_cloud_access=True) |
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.
Can defer to another PR - maybe we can update sky_check.check
in the future to return a list of enabled and disabled clouds
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.
Good point! Filed an issue for this #3570. Would prefer to leave it to the future, as the semantic for the return value should be discussed when clouds
argument is passed.
…unexist-cloud-in-candidate-resources
Closes #3565
Allow nonexistent cloud
With at least one cloud enabled
Without kubernetes and IBM enabled:
Master:
This PR:
For a dag:
With no cloud enabled
For a dag:
Speed up optimization
time sky launch task.yaml
master: 12.4s
this PR: 1.99s
Tested (run the relevant ones):
bash format.sh
ordered
andany_of
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
conda deactivate; bash -i tests/backward_compatibility_tests.sh