-
Notifications
You must be signed in to change notification settings - Fork 6k
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] add new sets of performance test for max_ongoing_requests=100
and doc change
#46399
[Serve] add new sets of performance test for max_ongoing_requests=100
and doc change
#46399
Conversation
Signed-off-by: Gene Su <[email protected]>
@GeneDer how much was the decrease? If it's substantial, we should reconsider the change, as we don't want people to benchmark the defaults and see a big decrease... When I previously did benchmarks I remember the perf. being very similar at 5 and 10 max concurrent requests |
HTTP went from 430ish to 300ish, ~30% drop |
This is not good. Can we run it w/ the default changed to 10 from 5? |
Also just paste screenshots, here to keep them in the history for easy reference in the future |
sg, let me get that running |
Signed-off-by: Gene Su <[email protected]>
From https://buildkite.com/ray-project/release/builds/18857#01907a5e-c295-4eef-b039-1bf6d79eff28 seems like setting max ongoing requests to 10 gives: |
…doc around using higher valuer for light weight requests Signed-off-by: Gene Su <[email protected]>
CI running here: but not sure it will properly post the status or not |
Signed-off-by: Gene Su <[email protected]>
@edoakes and I talked offline about the approach. We decided to add another set of tests to show the performance numbers of previous config when The release test ran on this branch shows the performance numbers at the previous and current |
max_ongoing_requests=100
for serve microbenchmarkmax_ongoing_requests=100
and doc change
Why are these changes needed?
Re: #45943 changed the default
max_ongoing_requests
down to 5 which caused the overall rps lower as will. This PR did two things:max_ongoing_requests =100
max_ongoing_requests
for lightweight requestsRelated issue number
Closes metrics dropping at https://b534fd88.us1a.app.preset.io/explore/?form_data_key=YWenQf8xEN9DHx7wmFVo3cRXYD-xoqObnK7HEC8J2Ho8FMnPCYM-d_as4EO7CG4b&dashboard_page_id=DCipe40Dxo&slice_id=1380
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.