Repository navigation
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 an issue where ENCODER_ONLY layers in hybrid models were incorrectly executing causal attention due to misleading metadata. By explicitly routing these layers to the encoder attention path, the implementation now correctly handles bidirectional attention for these components, improving output accuracy for pooling models. 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
|
47f2a6a to
35ae92f
Compare
|
👋 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:
[Attention][BugFix] Ensure ENCODER_ONLY layers in pooling models bypass causal pathSuggested PR Summary:
### What this PR does / why we need it?
This PR updates the attention forward logic for pooling models in `vllm_ascend`. Specifically, it ensures that `ENCODER_ONLY` layers (which are bidirectional) bypass the causal path and correctly use the encoder attention path, even when the metadata of a hybrid model indicates it is causal. This is necessary because `ENCODER_ONLY` layers never go through the `EncoderOnlyAttention` wrapper.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added unit tests in `tests/ut/attention/test_attention_v1.py` to verify that ENCODER_ONLY layers use the encoder path and DECODER layers keep the causal path.No review comments were provided, so there is no feedback to evaluate.
The pooling branch of AscendAttentionBackendImpl.forward chose the bidirectional path from attn_metadata.causal alone. ENCODER_ONLY layers of hybrid models (Gated DeltaNet + full attention, e.g. Qwen3.5-based D3) reach the backend with causal=True because they do not go through the EncoderOnlyAttention wrapper, so they silently ran causal attention. Also route layers whose attn_type is ENCODER_ONLY to _forward_encoder_attention, as upstream backends do. DECODER layers are unchanged. Add unit tests; the ENCODER_ONLY one fails without the change. Signed-off-by: do420 <denizozcanmc@gmail.com>
35ae92f to
3e5eb52
Compare
There was a problem hiding this comment.
The implementation matches upstream and covers the reported D3 regression. Before merge, can you confirm whether pooling + ENCODER_ONLY + PCP is supported? This path does not gather PCP K/V; if supported, please add a test or fail-fast, otherwise document the limitation.
The encoder attention path attends over rank-local K/V only; unlike the KV-cache write it never gathers the other PCP ranks' tokens, so bidirectional attention would silently be wrong. Pooling models default to the V1 model runner, which rejects PCP, but VLLM_USE_V2_MODEL_RUNNER=1 can force V2 and bypass that check. Fail at model construction instead, add unit tests for the guard and for the V1 rejection, and document the limitation. Signed-off-by: do420 <denizozcanmc@gmail.com>
Thanks for the input. pooling + ENCODER_ONLY + PCP is not supported: the encoder attention path only sees rank-local K/V and never gathers the other PCP ranks' tokens. |
bac467c to
376b204
Compare
Signed-off-by: Deniz Özcan <66875687+do420@users.noreply.github.com>
376b204 to
00681c7
Compare
|
/rerun Rerun (failed jobs only):
|
Stack the changes from vllm-project#19919 so the D3 numerical tests exercise the required ENCODER_ONLY attention behavior. Signed-off-by: do420 <denizozcanmc@gmail.com>
yiz-liu
left a comment
There was a problem hiding this comment.
Please remove unnecessary test cases.
Exercise the bidirectional dispatch for an ENCODER_ONLY pooling layer with causal metadata. Drop the extra decoder and prefill context parallelism cases per review. Signed-off-by: do420 <denizozcanmc@gmail.com>
|
/rerun Rerun (failed jobs only):
|
Align the branch with the current verified vLLM pin and CI checkout setup while retaining the focused encoder-only regression test. Signed-off-by: do420 <denizozcanmc@gmail.com>
The test cases are removed, only one kept. |
Signed-off-by: do420 <denizozcanmc@gmail.com>
|
/rerun Rerun (failed jobs only):
|
What this PR does / why we need it?
AscendAttentionBackendImpl.forwardchose the bidirectional pooling path fromattn_metadata.causalalone.ENCODER_ONLYlayers of hybrid models (Gated DeltaNet + full attention, e.g. the Qwen3.5-based Decision 3.0 / D3 checkpoints) reach the backend withcausal=True, because they do not go through theEncoderOnlyAttentionwrapper that clears the flag. These layers therefore silently ran causal attention even though the model setsconfig.is_causal = False.This PR also routes layers whose
attn_typeisENCODER_ONLYto_forward_encoder_attention, which is what upstream backends do for such layers regardless ofmetadata.causal.DECODERlayers are unchanged.causal=False(e.g. BERT-style models throughEncoderOnlyAttention) take the same branch as before.model_runner_typeis not"pooling").Prefill context parallelism (PCP) is not supported for
ENCODER_ONLYattention. The encoder attention path attends over rank-local K/V only. Unlike the KV-cache write, it never gathers the other PCP ranks' tokens (encoder-only layers have no KV cache). Pooling models default to the V1 model runner, which already rejects PCP, butVLLM_USE_V2_MODEL_RUNNER=1can force V2 and bypass that check, which would silently give wrong results. This PR therefore fails at model construction withNotImplementedErrorforENCODER_ONLYattention whenprefill_context_parallel_size > 1, and documents the limitation in the context parallel guide.Fixes #19918
Does this PR introduce any user-facing change?
Yes. Pooling models with
ENCODER_ONLYfull-attention layers in hybrid architectures now produce bidirectional (correct) results instead of causal ones, so their outputs change. No API, parameter or configuration change. Pooling models withENCODER_ONLYattention now also fail at startup under PCP (reachable only with the V2 runner forced), instead of silently computing with rank-local K/V. A release note fragment is included.How was this patch tested?
Unit test (
tests/ut/attention/test_attention_v1.py)test_forward_pooling_encoder_only_is_bidirectional_despite_causal_metadatachecks that a pooling layer withattn_type=ENCODER_ONLYuses the bidirectional attention kernel and returns its output even when the metadata sayscausal=True.The isolated CPU check passed with the fix and failed on the original backend because it never called the encoder kernel. After syncing with current
main(b93e8f711), the repository tests passed against vLLM24420b7a:python -m pytest -q tests/ut/attention/test_attention_v1.py tests/ut/test_platform.py— 203 passed, 1 skipped, in a CPU-only container with no NPU devices (2 CPUs, 12 GiB RAM).All manual-stage pre-commit hooks passed on the four PR-changed files.
bash format.sh ciwas also run across the whole repository; it reported two staged secret-scan false positives and two long-function warnings in files inherited unchanged frommain. The actual PR diff passed secret scanning. The earlier CI Gitleaks failure was caused by a missingorigin/mainref in the runner checkout; the branch is now synced with current main's CI setup.Ascend hardware (Ascend950DT, CANN 9.2.0.b080, torch_npu 2.10.0.post4, vLLM
c0b6f74, vLLM Ascend with the D3 adapter from #19804):vllm-sr/d3-edgeandvllm-sr/d3-nanowithrunner="pooling",max_model_len=4096. Eager runs had multimodal inputs enabled; graph runs had them limited to zero (graph-mode startup can also crash when a staletorch.compilecache written with a differentlimit_mm_per_promptsetting is reused; that is a known upstream vLLM issue, vllm-project/vllm#50891, and unrelated to this change). Compared against the checkpoint's reference runtime on the same prompts, softmax over each question's options (3 questions: choice, noul, score):Before the change the outputs matched a causal reference (0.0035) instead.
POST /v1/systemoneon d3-edge returns the same probabilities as the offline run after the change.Not covered
npu_fusion_attentionpath, which bidirectional attention requires.Checklist
Testing
Boundary / edge-case coverage
Regression test: the
ENCODER_ONLYunit test fails without the fix.Error / failure path coverage: the retained test focuses on the attention dispatch.
Meaningful & reliable tests: the regression test checks that the encoder kernel runs and its output is returned.
Performance impact: no benchmark attached, see "Not covered" above.
vLLM main: vllm-project/vllm@24420b7