Skip to content

[Jobs] --sync-down execution log support for managed jobs #4527

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Merged
merged 25 commits into from
Jan 9, 2025

Conversation

KeplerC
Copy link
Collaborator

@KeplerC KeplerC commented Jan 4, 2025

This aims to resolve #4056 for managed jobs, we should have a flag --sync-down that download the logs to local machine.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • manually tested
sky jobs logs --controller 7 --sync-down
sky jobs logs 7 --sync-down
sky jobs logs --name minimal --sync-down # download all of the job_ids using the same name
sky jobs logs --name minimal 7 --sync-down  #  error as expected 
  • smoke tests
pytest tests/test_smoke.py::test_managed_jobs_logs_sync_down

@KeplerC KeplerC marked this pull request as ready for review January 5, 2025 19:29
@KeplerC KeplerC marked this pull request as draft January 5, 2025 19:43
@KeplerC KeplerC marked this pull request as ready for review January 6, 2025 06:29
Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @KeplerC! Left several comments below : )

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @KeplerC! Left several comments : ) Also, we should add a smoke test for the newly added functionality

@Michaelvll Michaelvll requested a review from cg505 January 8, 2025 22:15
Copy link
Collaborator

@cg505 cg505 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good now, left some relatively minor comments.
Also, please address the lint failures!

Co-authored-by: Christopher Cooper <christopher@cg505.com>
- Changed the method name from `get_job_ids_by_name` to `get_all_job_ids_by_name` for clarity in `ManagedJobCodeGen`.
- Updated the CLI option flag for `sync-down` from `-d` to `-s` for better alignment with common conventions.
@cg505 cg505 self-requested a review January 9, 2025 18:25
Copy link
Collaborator

@cg505 cg505 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the changes, looks good to me!

@KeplerC KeplerC merged commit 1578108 into skypilot-org:master Jan 9, 2025
18 checks passed
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Jobs] Download job execution log.
3 participants