[BugFix][310p] Fix post-sampling not working in graph mode on 310P - #8017
Conversation
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical stability issue where 310P devices would hang during post-sampling due to an incompatible exponential operator kernel. By offloading the exponential distribution generation to the CPU and synchronizing streams appropriately, the service can now perform temperature-based sampling reliably without triggering internal NPU kernel errors. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[310P][Sampler][Feature] Implement AscendSampler310 and RejectionSampler supportSuggested PR Summary:
### What this PR does / why we need it?
This pull request introduces the `AscendSampler310` and `AscendTopKTopPSampler310` classes to support sampling operations specifically for the Ascend 310P hardware. It also integrates `RejectionSampler` into the `NPUModelRunner310`.
Feedback was provided regarding inefficient memory management and potential device mismatches in `sampler.py`. Specifically, the `_generate_exponential_q` function should be updated to handle device placement more robustly, and tensors should be initialized directly on the CPU to avoid unnecessary NPU allocations and blocking transfers when performing CPU-based exponential sampling.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
CI passed with existing tests.…llm-project#8017) ### What this PR does / why we need it? Enabling temperature in post-processing on 310P devices can cause the service to stall and eventually hang. We first traced the issue to a timeout where the temperature-related `div` operator was waiting for results from a sub-stream. After investigating the preceding operators, we finally identified the root cause as the `q.exponential_()` operator, which is not well supported on 310P and triggers an internal issue in the `add` kernel. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? This patch was thoroughly tested locally(accuracy-dataset test and stress test). It is not easy to design a proper unit test for this case, and I appreciate your understanding. - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@14acf42 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com> (cherry picked from commit b7987f9)
…llm-project#8017) ### What this PR does / why we need it? Enabling temperature in post-processing on 310P devices can cause the service to stall and eventually hang. We first traced the issue to a timeout where the temperature-related `div` operator was waiting for results from a sub-stream. After investigating the preceding operators, we finally identified the root cause as the `q.exponential_()` operator, which is not well supported on 310P and triggers an internal issue in the `add` kernel. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? This patch was thoroughly tested locally(accuracy-dataset test and stress test). It is not easy to design a proper unit test for this case, and I appreciate your understanding. - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@14acf42 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com> Signed-off-by: guxin108 <1252896542@qq.com>
…llm-project#8017) ### What this PR does / why we need it? Enabling temperature in post-processing on 310P devices can cause the service to stall and eventually hang. We first traced the issue to a timeout where the temperature-related `div` operator was waiting for results from a sub-stream. After investigating the preceding operators, we finally identified the root cause as the `q.exponential_()` operator, which is not well supported on 310P and triggers an internal issue in the `add` kernel. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? This patch was thoroughly tested locally(accuracy-dataset test and stress test). It is not easy to design a proper unit test for this case, and I appreciate your understanding. - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@14acf42 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
…llm-project#8017) ### What this PR does / why we need it? Enabling temperature in post-processing on 310P devices can cause the service to stall and eventually hang. We first traced the issue to a timeout where the temperature-related `div` operator was waiting for results from a sub-stream. After investigating the preceding operators, we finally identified the root cause as the `q.exponential_()` operator, which is not well supported on 310P and triggers an internal issue in the `add` kernel. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? This patch was thoroughly tested locally(accuracy-dataset test and stress test). It is not easy to design a proper unit test for this case, and I appreciate your understanding. - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@14acf42 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com> Signed-off-by: zouyida2052 <zouyida2002@gmail.com>
…llm-project#8017) ### What this PR does / why we need it? Enabling temperature in post-processing on 310P devices can cause the service to stall and eventually hang. We first traced the issue to a timeout where the temperature-related `div` operator was waiting for results from a sub-stream. After investigating the preceding operators, we finally identified the root cause as the `q.exponential_()` operator, which is not well supported on 310P and triggers an internal issue in the `add` kernel. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? This patch was thoroughly tested locally(accuracy-dataset test and stress test). It is not easy to design a proper unit test for this case, and I appreciate your understanding. - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@14acf42 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
…llm-project#8017) ### What this PR does / why we need it? Enabling temperature in post-processing on 310P devices can cause the service to stall and eventually hang. We first traced the issue to a timeout where the temperature-related `div` operator was waiting for results from a sub-stream. After investigating the preceding operators, we finally identified the root cause as the `q.exponential_()` operator, which is not well supported on 310P and triggers an internal issue in the `add` kernel. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? This patch was thoroughly tested locally(accuracy-dataset test and stress test). It is not easy to design a proper unit test for this case, and I appreciate your understanding. - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@14acf42 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com> Signed-off-by: nanxing <1014662416@qq.com>
…llm-project#8017) ### What this PR does / why we need it? Enabling temperature in post-processing on 310P devices can cause the service to stall and eventually hang. We first traced the issue to a timeout where the temperature-related `div` operator was waiting for results from a sub-stream. After investigating the preceding operators, we finally identified the root cause as the `q.exponential_()` operator, which is not well supported on 310P and triggers an internal issue in the `add` kernel. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? This patch was thoroughly tested locally(accuracy-dataset test and stress test). It is not easy to design a proper unit test for this case, and I appreciate your understanding. - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@14acf42 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
…llm-project#8017) ### What this PR does / why we need it? Enabling temperature in post-processing on 310P devices can cause the service to stall and eventually hang. We first traced the issue to a timeout where the temperature-related `div` operator was waiting for results from a sub-stream. After investigating the preceding operators, we finally identified the root cause as the `q.exponential_()` operator, which is not well supported on 310P and triggers an internal issue in the `add` kernel. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? This patch was thoroughly tested locally(accuracy-dataset test and stress test). It is not easy to design a proper unit test for this case, and I appreciate your understanding. - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@14acf42 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
…V2 on the 310P (#16503) ### What this PR does / why we need it? Enable **temperature / top-k / top-p** sampling on Ascend **310P Model Runner V2**. - First-version `Ascend310PSampler` only supported greedy (`argmax`) and rejected non-zero temperature. - Mainline MRV2 uses Triton Gumbel sampling; 310P has no Triton, and NPU `exponential_` / large RNG can hang under ACLGraph (same issue fixed in MRV1). - This PR reuses the **MRV1 inverse-CDF path** (`_random_sample_310p`: CPU uniform per request → NPU `softmax` + `cumsum` + `searchsorted`), while keeping the MRV2 `sampling_states` surface required by MTP draft `propose()`. - Still rejects min_p / penalties / logprobs / logit processors (follow-up). Refs: MRV1 post-sampling fixes (#8017 / #8077), CDF optimization (#12966). RFC: #15577 ### Does this PR introduce _any_ user-facing change? No. On 310P with `VLLM_USE_V2_MODEL_RUNNER=1`, requests may now use `temperature`, `top_k`, and `top_p` (previously raised `NotImplementedError`). Default greedy behavior is unchanged. ### How was this patch tested? - UT: `tests/ut/_310p/test_model_runner_v2_310p.py` (temperature accept / scale / greedy; still rejects penalties) - vLLM main: vllm-project/vllm@84030bb --------- Signed-off-by: Thiagor2002 <13476117628@163.com>
What this PR does / why we need it?
Enabling temperature in post-processing on 310P devices can cause the service to stall and eventually hang. We first traced the issue to a timeout where the temperature-related
divoperator was waiting for results from a sub-stream. After investigating the preceding operators, we finally identified the root cause as theq.exponential_()operator, which is not well supported on 310P and triggers an internal issue in theaddkernel.Does this PR introduce any user-facing change?
NA
How was this patch tested?
This patch was thoroughly tested locally(accuracy-dataset test and stress test). It is not easy to design a proper unit test for this case, and I appreciate your understanding.