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

[SkyServe] Combine Replica Log Files #2949

Merged
merged 13 commits into from
Jan 29, 2024

Conversation

dtran24
Copy link
Contributor

@dtran24 dtran24 commented Jan 6, 2024

Combines replica log files

  • Addresses [SkyServe] combine replica logs into one single file #2917
  • Edit: After being brought up with other maintainers, further discussion is required to determine whether another thread should be used to continuously sync down the local replica logs to the sky serve controller
  • Update (1/16/24): After syncing offline, we agreed that this approach of syncing down the logs on-demand would work for as a first-pass. Continuously syncing down the logs from the replica to the serve controller is not ideal since it consumes network resources from the controller. For the longer term, we want to have the replica logs written to a cloud's blob store (e.g. S3, GCS). This will require more thought and a design doc.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • 1) sky serve up vicuna.yaml -n vicuna. 2) Ran HTTP-direct commands to it. 3) Confirmed replica log file had correct content after running cancelling the replica from the sky-serve-controller.
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

@dtran24 dtran24 changed the title Combine Replica Log Files [SkyServe] Combine Replica Log Files Jan 13, 2024
@dtran24 dtran24 marked this pull request as ready for review January 25, 2024 02:52
Copy link
Collaborator

@cblmemo cblmemo 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 this feature!! It will be really helpful for organize the log files. Left several comments 🫡

Also, could you merge to the latest master branch?

sky/serve/serve_utils.py Outdated Show resolved Hide resolved
sky/serve/replica_managers.py Outdated Show resolved Hide resolved
sky/serve/serve_utils.py Outdated Show resolved Hide resolved
@dtran24 dtran24 requested a review from cblmemo January 25, 2024 03:36
@cblmemo
Copy link
Collaborator

cblmemo commented Jan 25, 2024

BTW are you merged to the latest master branch? There are some diff looks strange and I want to make sure if it is an auto-merging

@dtran24
Copy link
Contributor Author

dtran24 commented Jan 26, 2024

Just merged to the latest master branch

Copy link
Collaborator

@cblmemo cblmemo 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 fix!! Left several nits ;)

sky/serve/replica_managers.py Show resolved Hide resolved
sky/serve/serve_utils.py Outdated Show resolved Hide resolved
@dtran24 dtran24 requested a review from cblmemo January 29, 2024 04:17
sky/serve/replica_managers.py Outdated Show resolved Hide resolved
sky/serve/serve_utils.py Outdated Show resolved Hide resolved
@dtran24 dtran24 requested a review from cblmemo January 29, 2024 04:51
Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

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

Thanks!! LGTM🫡

sky/serve/replica_managers.py Outdated Show resolved Hide resolved
@dtran24
Copy link
Contributor Author

dtran24 commented Jan 29, 2024

Appreciate all the effort for reviewing the PR @cblmemo 🙇! Can't wait to have my first PR merged 😄

@cblmemo cblmemo merged commit 83a9dc3 into skypilot-org:master Jan 29, 2024
19 checks passed
@cblmemo cblmemo mentioned this pull request Feb 21, 2024
8 tasks
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.

None yet

2 participants