-
Notifications
You must be signed in to change notification settings - Fork 417
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] Add sky jobs logs --name
and fix sky job logs spinner
#3538
Conversation
sky/cli.py
Outdated
if name is not None: | ||
job_queue = managed_jobs.queue(refresh=False) | ||
job_id = None | ||
for job in job_queue: | ||
if job['job_name'] == name: | ||
job_id = job['job_id'] | ||
break | ||
if job_id is None: | ||
click.echo(f'Managed job with name {name!r} not found.') | ||
sys.exit(1) |
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.
To discuss: will this add too much overhead to the sky jobs logs
? Now it requires two SSH to the controller IIUC. Also, this seems not used in the smoke test?
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.
This might be fine as the code path will only be entered when name
is specified which is currently only used in smoke test?
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.
If a user does sky jobs logs --controller -n train-job
, then it will also hit this code path?
sky jobs logs --name
and fix sky job logs spinner
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 for adding this @Michaelvll ! Left some discussions :))
sky/jobs/utils.py
Outdated
from sky.jobs import utils | ||
from sky.jobs import constants as managed_job_constants | ||
from sky.jobs import state as managed_job_state | ||
from typing import Optional |
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.
why do we need this?
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.
This is needed as we inspect.getsource the function stream_logs
, which uses Optional in its signature. Moved this import to the later place.
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 for adding this feature @Michaelvll ! LGTM 🚀
This adds support for
sky jobs logs --name
, fixes #3599, and add test forsky job logs
This is to prevent issue like #3531 with the smoke test.
Tested (run the relevant ones):
bash format.sh
sky jobs launch -n test-log --cloud aws --gpus H100:8 echo hi
and check the output of the spinnersky launch -c test-output --cloud aws task.yaml
sky launch -c test-output --cloud gcp task.yaml
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
pytest tests/test_smoke.py::test_managed_jobs
pytest tests/test_smoke.py::test_managed_jobs --kubernetes
pytest tests/test_smoke.py::test_minimal
conda deactivate; bash -i tests/backward_compatibility_tests.sh