[CI/Build] Increasing timeout for multiproc worker tests #8203
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 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 do one of these:
🚀 |
|
/ready |
|
This PR fixes the "AMD Engine Test". The int argument of the join() operator defines the maximal count of seconds to wait for joining. Increasing this parameter doesn't result in the delay, rather it increases resilience of the tests that might be run on different machines, in different computational contexts, with different levels of host occupancy. |
|
/ready |
|
@njhill is increasing this timeout okay? |
|
@Alexei-V-Ivanov-AMD this test exercises the multiproc worker mechanics in isolation, with dummy worker impls. I don’t think the time difference it takes to start these dummy workers with spawn vs fork would be very significant? |
@njhill The experiment shows that it takes approximately 5-6 seconds to spawn those processes and less than a second if we fork them. So, 5 second timeout is not enough. https://docs.python.org/3.10/library/multiprocessing.html#contexts-and-start-methods |
|
From https://docs.python.org/3.10/library/multiprocessing.html#contexts-and-start-methods "Starting a process using this method [spawn] is rather slow compared to using fork or forkserver." |
|
just chiming in, if you cannot resolve the disagreement, you can introduce an env var like Lines 13 to 17 in de80783 |
|
@youkaichao I don't think this divergence between the platforms is necessary. As mentioned above, setting the time count to a reasonably large value will only affect our expectations on when the wait for the join operator becomes a burden. That way we only increase resilience of the test (something that is opposite to "flakiness", and we all want that test to be resilient). If join occurs faster, the execution continues without a delay. |
|
makes sense to me, let me chat with @njhill |
|
Hey sorry ... no disagreement and sorry didn't mean to make a big deal of this, I was just trying to understand because this should be a platform-agnostic test. My only hesitation to bumping the timeout that much higher is in case we make some change to the mechanics that causes it to unintentionally take much longer, then we wouldn't catch it. How about just increasing to something not quite as large, like 20 sec? |
IMHO it'll work fine. |
…t#8203) Signed-off-by: Alvant <[email protected]>
…t#8203) Signed-off-by: LeiWang1999 <[email protected]>
On ROCm we have to use spawn instead of fork and it creates an overhead, so five seconds might not be enough to create and destroy eight worker processes.
Please note, increasing the timeout to 60 seconds will not impact the test execution time unless there is a problem.
FIX #xxxx (link existing issues this PR will resolve)
BEFORE SUBMITTING, PLEASE READ THE CHECKLIST BELOW AND FILL IN THE DESCRIPTION ABOVE
PR Checklist (Click to Expand)
Thank you for your contribution to vLLM! Before submitting the pull request, please ensure the PR meets the following criteria. This helps vLLM maintain the code quality and improve the efficiency of the review process.
PR Title and Classification
Only specific types of PRs will be reviewed. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:
[Bugfix]for bug fixes.[CI/Build]for build or continuous integration improvements.[Doc]for documentation fixes and improvements.[Model]for adding a new model or improving an existing model. Model name should appear in the title.[Frontend]For changes on the vLLM frontend (e.g., OpenAI API server,LLMclass, etc.)[Kernel]for changes affecting CUDA kernels or other compute kernels.[Core]for changes in the core vLLM logic (e.g.,LLMEngine,AsyncLLMEngine,Scheduler, etc.)[Hardware][Vendor]for hardware-specific changes. Vendor name should appear in the prefix (e.g.,[Hardware][AMD]).[Misc]for PRs that do not fit the above categories. Please use this sparingly.Note: If the PR spans more than one category, please include all relevant prefixes.
Code Quality
The PR need to meet the following code quality standards:
format.shto format your code.docs/source/if the PR modifies the user-facing behaviors of vLLM. It helps vLLM user understand and utilize the new features or changes.Notes for Large Changes
Please keep the changes as concise as possible. For major architectural changes (>500 LOC excluding kernel/data/config/test), we would expect a GitHub issue (RFC) discussing the technical design and justification. Otherwise, we will tag it with
rfc-requiredand might not go through the PR.What to Expect for the Reviews
The goal of the vLLM team is to be a transparent reviewing machine. We would like to make the review process transparent and efficient and make sure no contributor feel confused or frustrated. However, the vLLM team is small, so we need to prioritize some PRs over others. Here is what you can expect from the review process:
action-requiredlabel on the PR if there are changes required. The contributor should address the comments and ping the reviewer to re-review the PR.Thank You
Finally, thank you for taking the time to read these guidelines and for your interest in contributing to vLLM. Your contributions make vLLM a great tool for everyone!