[Serve] log warning to reconfigure max_ongoing_requests if max_batch_size is less than max_ongoing_requests
#43840
Conversation
Signed-off-by: Gene Su <[email protected]>
Signed-off-by: Gene Su <[email protected]>
3c0bd0c
to
397899f
Compare
There was a problem hiding this comment.
This change won't solve the issue in isolation because the sender side (router) will still respect the limit.
The solution is also a little bit hacky to implicitly change the behavior like this -- I'd rather we do this at deployment time and update max_ongoing_requests then and log a warning.
Signed-off-by: Gene Su <[email protected]>
Signed-off-by: Gene Su <[email protected]>
Signed-off-by: Gene Su <[email protected]>
Signed-off-by: Gene Su <[email protected]>
Signed-off-by: Gene Su <[email protected]>
max_ongoing_requests if max_batch_size is less than max_ongoing_requests
|
@edoakes this is ready for another round of review. Changed to just log a warning and added a test for it |
python/ray/serve/_private/config.py
Outdated
|
|
||
| return method._get_max_batch_size() | ||
|
|
||
| def check_max_batch_size_bounded( |
There was a problem hiding this comment.
I think it makes more sense to this inside of serve.batching._BatchQueue rather than adding this method parsing hackery. As written, this currently does not work if users use the set_max_batch_size setter on the method (which can be done dynamically at runtime in the replica).
We'll need access to max_ongoing_requests there, for which I'd suggest we add deployment_config as a private attribute of ReplicaContext (and set/update it properly in replica.py for that purpose). This will likely be generally useful in the future anyways.
There was a problem hiding this comment.
Oh cool, didn't know there is a way for batching to get max_ongoing_requests. Thanks for the suggestion Ed!
Signed-off-by: Gene Su <[email protected]>
Signed-off-by: Gene Su <[email protected]>
Signed-off-by: Gene Su <[email protected]>
Signed-off-by: Gene Su <[email protected]>
|
@edoakes this is ready for review |
|
@edoakes this is ready for merge |
…ch_size` is less than `max_ongoing_requests` (ray-project#43840) Our existing request rate limiting is not factor in the batch size. When using a large max_batch_size (> max_ongoing_requests), the batch won't process the batch up to max_batch_size even when batch_wait_timeout_s is set large enough to collect requests. This PR look for each and every methods on the user code to find the max of max_batch_size on all methods and log a warning to suggest user to reconfigure max_ongoing_requests in order to reach max_batch_size limit. --------- Signed-off-by: Gene Su <[email protected]>
Why are these changes needed?
Our existing request rate limiting is not factor in the batch size. When using a large
max_batch_size(>max_ongoing_requests), the batch won't process the batch up tomax_batch_sizeeven whenbatch_wait_timeout_sis set large enough to collect requests. This PR look for each and every methods on the user code to find the max ofmax_batch_sizeon all methods and log a warning to suggest user to reconfiguremax_ongoing_requestsin order to reachmax_batch_sizelimit.Related issue number
Closes #43288
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.