[BugFix][310P] Fall back for unsupported penalty kernels - #15235
UniversePeak wants to merge 7 commits into
Conversation
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 compatibility issue where Triton-Ascend penalty kernels were incorrectly invoked on Ascend 310P hardware, leading to sampling failures. By updating the sampler and rejection sampler logic to detect 310P devices, the system now correctly falls back to the standard vLLM penalty implementation, ensuring stability for users of these devices while preserving optimized performance elsewhere. Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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. Tip 💡 Consider Linking a Related Issue or RFCYour PR title contains the [BugFix] tag, indicating a bug fix or new feature. Linking a related issue or RFC in the PR description is strongly encouraged — it gives reviewers helpful context and speeds up the review. You can use any of these keywords:
🙏 Thanks for helping us keep the project well-organized! |
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Ops][BugFix] Fallback to vLLM penalties on 310P and fix rejection sampler shape mismatchSuggested PR Summary:
### What this PR does / why we need it?
This pull request introduces a fallback to the default vLLM penalty implementation on Ascend 310P devices because Triton-Ascend penalty kernels are not supported on this hardware. This fallback is implemented in both the standard sampler and the rejection sampler.
However, a critical issue was identified in the rejection sampler fallback: calling `Sampler.apply_penalties` directly with the original `sampling_metadata` will cause a `RuntimeError` due to shape mismatch. In speculative decoding, the `logits` tensor is expanded, but the penalty tensors are not. To resolve this, the penalty tensors in `sampling_metadata` must be repeated using `repeat_indices` before invoking the fallback.
### Does this PR introduce _any_ user-facing change?
No, this is an internal fallback mechanism for Ascend 310P devices to ensure compatibility and prevent execution failures.
### How was this patch tested?
The changes are covered by new unit tests in `tests/ut/test_penalties_310p.py` which mock the device type and Triton availability to verify that the fallback is correctly triggered.| if not HAS_TRITON: | ||
| # Triton-Ascend penalty kernels are not supported on 310P. Keep the | ||
| # vLLM implementation for this device even when Triton is installed. | ||
| if not HAS_TRITON or is_310p(): |
There was a problem hiding this comment.
In speculative decoding (rejection sampler), the logits tensor has an expanded shape of [num_tokens + batch_size, vocab_size] to account for all draft and bonus tokens, whereas the penalty tensors in sampling_metadata (such as presence_penalties, frequency_penalties, etc.) have a shape of [batch_size].\n\nDirectly calling Sampler.apply_penalties(logits, sampling_metadata, output_token_ids) without repeating/expanding the penalty tensors using repeat_indices will result in a guaranteed runtime RuntimeError due to shape mismatch on 310P devices.\n\nTo fix this, we should create a copy of sampling_metadata with the penalty tensors repeated by repeat_indices before passing it to Sampler.apply_penalties. This also allows us to update the warning message to accurately reflect that Triton is unsupported on 310P (matching the updated warning in sampler.py).\n\nHere is the suggested fix for this block:\n\npython\n if not HAS_TRITON or is_310p():\n logger.warning_once(\n "[sample/rejection_sampler] Triton unavailable or unsupported on 310P, falling back to vLLM default "\n "penalty implementation in rejection sampler. Rejection sampling performance "\n "may be degraded on NPU. "\n )\n repeated_sampling_metadata = replace(\n sampling_metadata,\n prompt_token_ids=sampling_metadata.prompt_token_ids[repeat_indices] if sampling_metadata.prompt_token_ids is not None else None,\n presence_penalties=sampling_metadata.presence_penalties[repeat_indices],\n frequency_penalties=sampling_metadata.frequency_penalties[repeat_indices],\n repetition_penalties=sampling_metadata.repetition_penalties[repeat_indices],\n )\n return Sampler.apply_penalties(logits, repeated_sampling_metadata, output_token_ids)\n
4231e87 to
9aa52cf
Compare
|
The code and pre-commit checks are passing, but the current |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Resolved the merge conflict by rebasing the branch onto the latest |
|
Correction to my previous update: the branch was updated by merging the latest |
752e272 to
67a95fa
Compare
|
The latest workflow run for the current head |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Rebased onto the latest The PR is mergeable again. Since |
6a22a52 to
68a96e3
Compare
Triton-Ascend penalty kernels are unsupported on 310P, but the sampler selected them whenever Triton was installed. Use the upstream penalty implementation on 310P for presence, frequency, and repetition penalties, including speculative decoding. Signed-off-by: UniversePeak <UniversePeak@users.noreply.github.com>
Repeat prompt and penalty tensors for speculative logits before calling the upstream vLLM fallback, and cover the expanded multi-request shape in the 310P regression test. Signed-off-by: UniversePeak <UniversePeak@users.noreply.github.com>
Use the hardware profile capability model introduced on main while preserving the 310P Triton penalty fallback. Keep rejection sampler penalty metadata expanded with repeat_indices before calling the upstream fallback. Signed-off-by: UniversePeak <UniversePeak@users.noreply.github.com>
Signed-off-by: UniversePeak <UniversePeak@users.noreply.github.com>
…n is possible The fallback path of AscendRejectionSampler.apply_penalties previously indexed sampling_metadata.prompt_token_ids unconditionally. Metadata without per-request penalty tensors (no_penalties edge cases and the existing UT contract) must be handed to vLLM's Sampler.apply_penalties as-is, matching the upstream behavior. Signed-off-by: UniversePeak <UniversePeak@users.noreply.github.com>
…lace apply_logits_processors also routes through the penalty fallback, where sampling_metadata may be a plain namespace (as in the existing UTs). Duplicate the metadata as a SimpleNamespace in that case instead of calling dataclasses.replace. Signed-off-by: UniversePeak <UniversePeak@users.noreply.github.com>
68a96e3 to
15de9fc
Compare
|
The E2E run exposed a real bug in my fallback: Fixed in 68a96e3: the metadata is now cloned by re-instantiating the container with expanded penalty tensors (falling back to the original metadata when no expansion is needed), so the same fix now covers both the regular sampling path and the rejection sampler path. Focused checks pass locally; the PR branch and fork copy are in sync. |
fdc3f4f to
6a70bf6
Compare
…lback Signed-off-by: UniversePeak <UniversePeak@users.noreply.github.com>
6a70bf6 to
f2ffe3f
Compare
|
The CPU unit test failure is fixed by 68a96e3: on the latest E2E run, |
What this PR does / why we need it?
Fixes #15161. Triton-Ascend penalty kernels are not supported on Ascend 310P, but vllm-ascend v0.23.0 selected them whenever Triton was available. This causes requests using presence_penalty/frequency_penalty/repetition_penalty to fail during sampling.
The regular and speculative samplers now use the upstream vLLM penalty implementation on 310P while retaining the Triton path on other devices.
Does this PR introduce any user-facing change?
Yes. Presence, frequency, and repetition penalties no longer route to unsupported Triton kernels on 310P.
How was this patch tested?
tests/ut/_310p/sample/test_penalties_310.pycovering regular and speculative sampler routing.ruff check --fix vllm_ascend/sample/sampler.py vllm_ascend/sample/rejection_sampler.py tests/ut/_310p/sample/test_penalties_310.pyruff format --check vllm_ascend/sample/sampler.py vllm_ascend/sample/rejection_sampler.py tests/ut/_310p/sample/test_penalties_310.pypython -m py_compile vllm_ascend/sample/sampler.py vllm_ascend/sample/rejection_sampler.py tests/ut/_310p/sample/test_penalties_310.pygit diff --checkThe full pytest environment could not load because the available host has vllm 0.26.0 without the repository-required
vllm._C,uvloop, andtorch_npudependencies; 310P hardware validation is left to CI/maintainer hardware.Signed-off-by: UniversePeak UniversePeak@users.noreply.github.com