-
Notifications
You must be signed in to change notification settings - Fork 460
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
[Logging] Symlink to latest logs #3769
base: master
Are you sure you want to change the base?
[Logging] Symlink to latest logs #3769
Conversation
9fcf425
to
b967306
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 @ShreyasKallingal. Took a quick look and tested it a bit. Ran into an issue with sky launch
on my mac, see comment for logs.
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 @ShreyasKallingal! Tried it and works nicely. Left some comments on the code.
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 @ShreyasKallingal! some minor comments, otherwise LGTM!
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 @ShreyasKallingal! We want to take care of the concurrency issue below. :)
I grouped the symlink removal with creation in a critical section. And refactored to add a fast path for logging to |
Resolves #3617
Tested (run the relevant ones):
bash format.sh
pytest tests/test_smoke.py::test_fill_in_the_name
New smoke test
pytest tests/test_smoke.py::test_symlink_latest_logs
Manual steps:
sky local up
or somesky launch/exec
command (doesn't matter if it succeeds or not -- just need to check log output)ls -l ~/sky_logs