Skip to content
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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

ShreyasKallingal
Copy link

@ShreyasKallingal ShreyasKallingal commented Jul 21, 2024

Resolves #3617

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name

New smoke test

pytest tests/test_smoke.py::test_symlink_latest_logs

Manual steps:

  1. Run sky local up or some sky launch/exec command (doesn't matter if it succeeds or not -- just need to check log output)
  2. Observe symlink "sky_latest" via ls -l ~/sky_logs
  3. Repeat and confirm that the symlink points to the latest logs each time

@ShreyasKallingal ShreyasKallingal marked this pull request as ready for review July 21, 2024 05:27
@ShreyasKallingal ShreyasKallingal marked this pull request as draft July 22, 2024 02:28
@ShreyasKallingal ShreyasKallingal marked this pull request as ready for review July 22, 2024 03:44
Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a 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.

sky/utils/log_utils.py Outdated Show resolved Hide resolved
sky/utils/log_utils.py Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
sky/utils/log_utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a 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.

tests/test_smoke.py Outdated Show resolved Hide resolved
tests/test_smoke.py Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
sky/utils/log_utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a 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!

sky/utils/log_utils.py Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
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 for adding this @ShreyasKallingal! We want to take care of the concurrency issue below. :)

sky/utils/log_utils.py Outdated Show resolved Hide resolved
@ShreyasKallingal
Copy link
Author

I grouped the symlink removal with creation in a critical section. And refactored to add a fast path for logging to /dev/null. Thanks for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logging] Symlink latest logs to a latest path
3 participants