-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[serve] Deprecate system-level batching with warning, update the docs #14648
Conversation
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.
Looks great, thanks for the careful doc update! No major comments
doc/source/serve/tutorials/batch.rst
Outdated
seconds to wait for a full batch to arrive. The default wait is ``0s`` to | ||
minimize query latency. You can increase the timeout to improve throughput | ||
and increase utilization at the cost of some additional latency. | ||
By default, Ray Serve performs *opportunistic batching*. This means that as |
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.
This note was the most useful piece of the documentation for me when I was first reading it (though maybe it's very standard and not that useful to others). Is there a way to make it more prominent/move it higher up without disrupting the flow?
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.
I think it might be fine to just put it all the way at the top after the top-level description of how to do batching. Thoughts?
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.
I moved it just after the initial code sample, I think that's an improvement. Any earlier is probably confusing. LMK what you think.
Co-authored-by: architkulkarni <architkulkarni@users.noreply.github.com>
Co-authored-by: architkulkarni <architkulkarni@users.noreply.github.com>
Co-authored-by: architkulkarni <architkulkarni@users.noreply.github.com>
Co-authored-by: architkulkarni <architkulkarni@users.noreply.github.com>
Thanks for the review @architkulkarni. Addressed the comments, please have another look. |
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.
Changes look good, thanks!
Why are these changes needed?
Deprecates the existing system-level batching codepath and instead recommends
@serve.batch
. The old codepath will still work after this PR, but print a warning pointing at the docs for the new workflow.Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.