[jobs] catch unimportant errors in controller proccess #4615
Conversation
cg505
force-pushed
the
jobs-preemption-handling
branch
from
4dee032 to
5d480f0
Compare
|
/quicktest-core |
|
/smoke-test --managed-jobs |
sky/jobs/controller.py
Outdated
| logger.info( | ||
| 'The user job failed. Please check the logs below.\n' | ||
| f'== Logs of the user job (ID: {self._job_id}) ==\n') | ||
|
|
||
| self._download_log_and_stream(task_id, handle) | ||
| try: | ||
| self._download_log_and_stream(task_id, handle) |
There was a problem hiding this comment.
Seem we already have the error handling in the function. Should we handle it again here?
There was a problem hiding this comment.
The handling inside the function only handles some case. I think we should catch a broader exception here. I will check if we can move the handling inside the function without breaking skyserve usage.
sky/jobs/controller.py
Outdated
| end_time = managed_job_utils.get_job_timestamp( | ||
| self._backend, cluster_name, get_end_time=True) |
There was a problem hiding this comment.
Should we have the function to handle the exceptions inside?
There was a problem hiding this comment.
That seems counterintuitive, could break some other use of the function that would not expect this fallback behavior.
There was a problem hiding this comment.
Well, it seems this get_job_timestamp is only called twice in this file, and both of them needs the exception handling with the same logic. We can probably rename the function to something like try_to_get_job_timestamp_or_current_time and have the exception handling inside?
After the managed job completes, there are a few cases where we still try to access the cluster. If that fails, don't crash the controller.
Tested (run the relevant ones):
bash format.shpytest tests/test_smoke.pypytest tests/test_smoke.py::test_fill_in_the_nameconda deactivate; bash -i tests/backward_compatibility_tests.sh