Skip to content

[data.llm][Bugfix] Fix doc to only support int concurrency#54196

Merged
kouroshHakha merged 6 commits intoray-project:masterfrom
lk-chen:batch_llm_concurrency
Jul 2, 2025
Merged

[data.llm][Bugfix] Fix doc to only support int concurrency#54196
kouroshHakha merged 6 commits intoray-project:masterfrom
lk-chen:batch_llm_concurrency

Conversation

@lk-chen
Copy link
Copy Markdown
Contributor

@lk-chen lk-chen commented Jun 28, 2025 •
edited by kouroshHakha
Loading

Why are these changes needed?

This PR removes tuple concurrency support from API doc

Closes https://anyscale1.atlassian.net/browse/CI-1155

Related issue number

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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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 :(

@lk-chen
[data.llm] allow tuple concurrency
2851229 
Signed-off-by: Linkun <github@lkchen.net>
Copilot AI review requested due to automatic review settings June 28, 2025 07:49
@lk-chen lk-chen requested a review from a team as a code owner June 28, 2025 07:49
Copilot AI reviewed Jun 28, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug related to the handling of the tuple concurrency configuration in the vLLM engine processor. The changes ensure that when a tuple is provided, the corresponding min and max sizes are set correctly.

  • Updated the test case to pass a tuple for concurrency.
  • Modified the processor configuration to correctly extract min and max values from a tuple.
  • Adjusted ActorPoolStrategy parameters to reflect fixed or auto‑scaled concurrency.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
release/llm_tests/batch/test_batch_vllm.py Updated test input to validate tuple concurrency configuration
python/ray/llm/_internal/batch/processor/vllm_engine_proc.py Fixed the processor configuration logic to respect tuple concurrency
Comments suppressed due to low confidence (1)

release/llm_tests/batch/test_batch_vllm.py:61

  • [nitpick] Consider updating the inline comment to accurately reflect that a tuple is provided for concurrency, such as 'concurrency=(1, 2)'.
        (1, 2, (1, 2)),  # PP=2, concurrency=2

python/ray/llm/_internal/batch/processor/vllm_engine_proc.py Outdated
min_size=config.concurrency
if isinstance(config.concurrency, int)
else processor_concurrency[0],
max_size=processor_concurrency[1],
Copy link

Copilot AI Jun 28, 2025

Choose a reason for hiding this comment

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

[nitpick] For improved readability, consider refactoring the inline conditional used to derive min_size into a separate variable.

Copilot uses AI. Check for mistakes.
@lk-chen
better format
97027b2 
Signed-off-by: Linkun <github@lkchen.net>
kouroshHakha
kouroshHakha reviewed Jul 2, 2025
View reviewed changes
python/ray/llm/_internal/batch/processor/vllm_engine_proc.py
compute=ray.data.ActorPoolStrategy(
min_size=config.concurrency,
max_size=config.concurrency,
# vLLM start up time is significant, so if user give fixed
Copy link
Copy Markdown
Contributor

@kouroshHakha kouroshHakha Jul 2, 2025

Choose a reason for hiding this comment

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

Following up on https://anyscale1.atlassian.net/browse/CI-1155?focusedCommentId=35311&sourceType=mention?atlOrigin%3D93e50a61c54043ee873dca7979b3f9cd

We should 1) update the doc for concurrency that (m,n) is not supported. 2) raise a validation error at config level if anything but an int is passed in.

@lk-chen
fix doc
aabe9f1 
Signed-off-by: Linkun <github@lkchen.net>
@lk-chen lk-chen changed the title [data.llm][Bugfix] Respect tuple concurrency config [data.llm][Bugfix] Fix doc to only support int concurrency Jul 2, 2025
lk-chen added 3 commits July 2, 2025 15:22
@lk-chen
lint
5bb84bb 
Signed-off-by: Linkun <github@lkchen.net>
@lk-chen
fix test
58765ca 
Signed-off-by: Linkun <github@lkchen.net>
@lk-chen
add model source
4b63c52 
Signed-off-by: Linkun <github@lkchen.net>
@lk-chen lk-chen requested a review from kouroshHakha July 2, 2025 23:01
@kouroshHakha kouroshHakha added the go add ONLY when ready to merge, run all tests label Jul 2, 2025
@kouroshHakha kouroshHakha reopened this Jul 2, 2025
@kouroshHakha kouroshHakha enabled auto-merge (squash) July 2, 2025 23:11
@kouroshHakha kouroshHakha merged commit 6e736d5 into ray-project:master Jul 2, 2025
6 of 7 checks passed
@lk-chen lk-chen deleted the batch_llm_concurrency branch July 3, 2025 03:36
elliot-barn pushed a commit that referenced this pull request Jul 7, 2025
@lk-chen @elliot-barn
[data.llm][Bugfix] Fix doc to only support int concurrency (#54196)
c83b67f 
Signed-off-by: Linkun <github@lkchen.net>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
@oxiez oxiez mentioned this pull request Aug 12, 2025
Closed
@axreldable axreldable mentioned this pull request Aug 24, 2025
Merged
8 tasks
dstrodtman pushed a commit to dstrodtman/ray that referenced this pull request Oct 6, 2025
@lk-chen @dstrodtman
[data.llm][Bugfix] Fix doc to only support int concurrency (ray-pro…
ac43f04 
…ject#54196)

Signed-off-by: Linkun <github@lkchen.net>
Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@kouroshHakha kouroshHakha kouroshHakha approved these changes

Assignees

No one assigned

Labels

go add ONLY when ready to merge, run all tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lk-chen @kouroshHakha