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

Enable Log Rotation on Serve #31844

Merged
merged 4 commits into from
Jan 27, 2023

Conversation

andreapiso
Copy link
Contributor

Why are these changes needed?

This PR adds log rotation for Ray Serve, letting it inherit rotation parameters (max_bytes, backup_count) from Ray Core, bringing a more consistent logging experience to Ray (as opposed to having the serve/ folder grow forever while the other logs rotate.

Related issue number

Closes #31842

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: Andrea Pisoni <andreapiso@gmail.com>
Signed-off-by: Andrea Pisoni <andreapiso@gmail.com>
@andreapiso
Copy link
Contributor Author

@edoakes would you be willing to have a look?

Copy link
Contributor

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

@andreapiso thanks so much for the contribution, this looks great!

Left a small comment and would like to have @rkooo567 take a quick look because he worked on the Ray-level log rotation and may have a comment or two.

Comment on lines 54 to 55
max_bytes = ray._private.worker._global_node.max_bytes
backup_count = ray._private.worker._global_node.backup_count
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could you add these as parameters to this function and if they aren't provided pick them up from the ray variables?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Also we need to update a doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will add these parameters to the function, defaulting to None (where if the params are None, they will be taken from the ray global node), and update the docs since the function signature will change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let us know when this is done!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! As per the doc, i made a brief addition on the Serve Logging documentaiton, pointing to the original Log Rotation page on Ray Core.

@edoakes edoakes added @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Jan 23, 2023
Signed-off-by: Andrea Pisoni <andreapiso@gmail.com>
Signed-off-by: Andrea Pisoni <andreapiso@gmail.com>
@edoakes
Copy link
Contributor

edoakes commented Jan 25, 2023

@ray-project/ray-docs please take a look at the docs changes here

@edoakes edoakes added docs An issue or change related to documentation and removed @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. labels Jan 25, 2023
@rkooo567
Copy link
Contributor

@edoakes there's a serve failure. can you see if this is relevant?

@andreapiso
Copy link
Contributor Author

@edoakes there's a serve failure. can you see if this is relevant?

I see the same timeout/OOM error on other PRs - so maybe the test is flaky?

E.g. #31972 --> https://buildkite.com/ray-project/oss-ci-build-pr/builds/10466#0185efec-d772-4e18-8b51-d6547526c8d1

@rkooo567 rkooo567 merged commit 7b2299b into ray-project:master Jan 27, 2023
@rkooo567
Copy link
Contributor

Thanks for the contribution!

edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
This PR adds log rotation for Ray Serve, letting it inherit rotation parameters (max_bytes, backup_count) from Ray Core, bringing a more consistent logging experience to Ray (as opposed to having the serve/ folder grow forever while the other logs rotate.

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs An issue or change related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Serve] Add support for log rotation
4 participants