-
Notifications
You must be signed in to change notification settings - Fork 510
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] Catch any exception for the spot queue fetching failure #1757
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.
Should we catch KeyboardInterrupt as well? To repro: run status
, and immediately ctrl-c (may need to try a few times with different timing). On master it'd show a long stacktrace then KeyboardInterrupt.
Good point! Fixed the KeyboardInterrupt related to the multiprocessing, but will leave the KeyboardInterrupt for other parts of the code to the future PR, as we may want to use Another fix added: our previous use of the |
def get_use_default_catalog() -> bool: | ||
if not hasattr(_thread_local_config, 'use_default_catalog'): | ||
_thread_local_config.use_default_catalog = False |
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.
Maybe copy the other file's comments here too.
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! Added. Thanks!
# down, and the hint for showing sky spot queue | ||
# will still be shown. | ||
num_in_progress_jobs = -1 | ||
msg = 'KeyboardInterrupt' |
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.
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.
Weirdly, I cannot reproduce the problem shown in the figure, but the keyboard interruption handling here is indeed a bit tricky as it involves multiple processes. Let's leave it for a future PR. : )
…t-org#1757) * Catch any exception for the spot queue fetching failure. * Fix keyboard interrupt * lint * Add comment
…t-org#1757) * Catch any exception for the spot queue fetching failure. * Fix keyboard interrupt * lint * Add comment
This is to fix the issue when the
sky status
meet other exceptions during the spot job query. It could happen when the current active account is different from the one used for the spot controller.Previous:
Now:
Tested (run the relevant ones):
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
bash tests/backward_comaptibility_tests.sh