-
Notifications
You must be signed in to change notification settings - Fork 539
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
[UX] Optimizing error message when disabled cloud storage attempted #1858
[UX] Optimizing error message when disabled cloud storage attempted #1858
Conversation
…capepainter/skypilot into disabled_cloud_ux_optimize merge
0d5c288
to
6ee3952
Compare
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 @landscapepainter! Left some comments.
@romilbhardwaj Ready for another look :)) |
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 @landscapepainter! Left some comments.
sky/global_user_state.py
Outdated
if not isinstance(cloud, clouds.Lambda) and | ||
not isinstance(cloud, clouds.IBM) |
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 we:
- Update
STORE_ENABLED_CLOUDS
in storage.py to a) be a List[str] and b) Add cloudflare.NAME to this list? - Change this list comprehension to select only the clouds in
STORE_ENABLED_CLOUDS
?
Trying to minimize the number of places the list of storage enabled clouds needs to be maintained :)
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.
Updated.
Note the change I made in task.py/get_preferred_store_type()
as STORE_ENABLED_CLOUDS
is List[str]
now.
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.
@romilbhardwaj Ready for another look!
…capepainter/skypilot into disabled_cloud_ux_optimize update tip
@romilbhardwaj Updated :) |
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 @landscapepainter! Should be good to go if test_smoke.py::TestStorageWithCredentials
also passes.
Passing |
This PR resolves #1831
When a Cloud Storage was set in the entrypoint yaml without enabling the cloud, Skypilot printed out long stack trace of error message. These messages are condensed into a simple message while removing the stack trace.
Before:
After:
Before:
After:
Tested (run the relevant ones):