Skip to content

[V1] [KVConnector] Fix MultiprocExecutor worker output aggregation#21048

Merged
DarkLight1337 merged 2 commits intovllm-project:mainfrom
sdavidbd:fix/worker_output_aggregation
Jul 17, 2025
Merged

[V1] [KVConnector] Fix MultiprocExecutor worker output aggregation#21048
DarkLight1337 merged 2 commits intovllm-project:mainfrom
sdavidbd:fix/worker_output_aggregation

Conversation

@sdavidbd
Copy link
Contributor

@sdavidbd sdavidbd commented Jul 16, 2025

Purpose

Fix an issue in MultiprocExecutor where the finished_sending and finished_recving fields in aggregated worker outputs were not correctly updated. This could wrongly propagate some requests as finished even when certain workers had not reported them yet.

This PR also adds comprehensive unit tests for _aggregate_workers_output and _async_aggregate_workers_output to ensure correct behavior in various scenarios.

Test Plan

Run the new unit tests:

$ pytest -s -v tests/v1/executor/test_multiproc_executor.py

Test Result

All tests pass. Example output:

============================================================================================ test session starts =============================================================================================
platform linux -- Python 3.12.9, pytest-8.3.5, pluggy-1.6.0 -- /home/pliops/miniconda3/envs/dbd-env/bin/python3.12
cachedir: .pytest_cache
rootdir: /home/pliops/workspace/davidb/git/vllm-dbd
configfile: pyproject.toml
plugins: anyio-4.9.0
collected 2 items

tests/v1/executor/test_multiproc_executor.py::test_aggregate_workers_output PASSED
tests/v1/executor/test_multiproc_executor.py::test_async_aggregate_workers_output PASSED

============================================================================================= 2 passed in 0.30s ==============================================================================================

@gemini-code-assist
Copy link
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

@sdavidbd sdavidbd force-pushed the fix/worker_output_aggregation branch from 219abfe to 2aa7589 Compare July 16, 2025 12:25
David Ben-David added 2 commits July 16, 2025 17:53
Signed-off-by: David Ben-David <davidb@pliops.com>
Signed-off-by: David Ben-David <davidb@pliops.com>
@sdavidbd sdavidbd force-pushed the fix/worker_output_aggregation branch from 2aa7589 to 042a898 Compare July 16, 2025 14:53
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @sdavidbd, and especially for adding these missing tests.

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Jul 16, 2025
@njhill njhill mentioned this pull request Jul 16, 2025
4 tasks
@DarkLight1337 DarkLight1337 merged commit 4fcef49 into vllm-project:main Jul 17, 2025
73 checks passed
x22x22 pushed a commit to x22x22/vllm that referenced this pull request Aug 5, 2025
…llm-project#21048)

Signed-off-by: David Ben-David <davidb@pliops.com>
Co-authored-by: David Ben-David <davidb@pliops.com>
Signed-off-by: x22x22 <wadeking@qq.com>
Pradyun92 pushed a commit to Pradyun92/vllm that referenced this pull request Aug 6, 2025
…llm-project#21048)

Signed-off-by: David Ben-David <davidb@pliops.com>
Co-authored-by: David Ben-David <davidb@pliops.com>
npanpaliya pushed a commit to odh-on-pz/vllm-upstream that referenced this pull request Aug 6, 2025
…llm-project#21048)

Signed-off-by: David Ben-David <davidb@pliops.com>
Co-authored-by: David Ben-David <davidb@pliops.com>
jinzhen-lin pushed a commit to jinzhen-lin/vllm that referenced this pull request Aug 9, 2025
…llm-project#21048)

Signed-off-by: David Ben-David <davidb@pliops.com>
Co-authored-by: David Ben-David <davidb@pliops.com>
Signed-off-by: Jinzhen Lin <linjinzhen@hotmail.com>
paulpak58 pushed a commit to paulpak58/vllm that referenced this pull request Aug 13, 2025
…llm-project#21048)

Signed-off-by: David Ben-David <davidb@pliops.com>
Co-authored-by: David Ben-David <davidb@pliops.com>
Signed-off-by: Paul Pak <paulpak58@gmail.com>
diegocastanibm pushed a commit to diegocastanibm/vllm that referenced this pull request Aug 15, 2025
…llm-project#21048)

Signed-off-by: David Ben-David <davidb@pliops.com>
Co-authored-by: David Ben-David <davidb@pliops.com>
Signed-off-by: Diego-Castan <diego.castan@ibm.com>
epwalsh pushed a commit to epwalsh/vllm that referenced this pull request Aug 27, 2025
…llm-project#21048)

Signed-off-by: David Ben-David <davidb@pliops.com>
Co-authored-by: David Ben-David <davidb@pliops.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants