[BugFix][MRV2] Use PCP-local graph capture buffers on v0.28.0 - #16179
zhao-stack wants to merge 7 commits into
Conversation
Signed-off-by: shenzhao <shenzhao9@huawei.com>
|
/e2e tests/e2e/pull_request/four_card/context_parallel/test_accuracy_v2.py::test_dsv3_2_sfa_pcp_graph_diagnostic |
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 introduces temporary diagnostic instrumentation to investigate accuracy failures observed in the v0.28.0 MRV2 SFA PCP configuration. By injecting custom logging into the model runner, the changes allow for the collection of granular token-level data and logit margins under different graph execution modes. This work is strictly for diagnostic purposes to isolate potential buffer ownership discrepancies and does not include any production-level fixes. 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Test][Feature] Add graph diagnostic test for DSV3.2 SFA PCPSuggested PR Summary:
### What this PR does / why we need it?
This PR adds a new end-to-end diagnostic test `test_dsv3_2_sfa_pcp_graph_diagnostic` to compare graph execution modes and collect raw logits. Feedback was provided to update the patched `logged_generate` function signature to accept and forward `*args` and `**kwargs` to ensure compatibility with the original `VllmRunner.generate_greedy` method.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
This is a test addition, which can be run via pytest.| def logged_generate(runner, prompts, max_tokens): | ||
| if not collect_logits: | ||
| outputs = original_generate(runner, prompts, max_tokens) | ||
| records = [ | ||
| {"prompt": prompt, "full_token_ids": ids, "full_text": text} | ||
| for prompt, (ids, text) in zip(prompts, outputs, strict=True) | ||
| ] | ||
| else: | ||
| scored_outputs = runner.generate_greedy_logprobs(prompts, max_tokens, 5) |
There was a problem hiding this comment.
The patched logged_generate function does not accept *args or **kwargs, which makes it incompatible with the signature of VllmRunner.generate_greedy. If generate_greedy is called with any additional arguments (such as images, videos, audios, or other keyword arguments), a TypeError will be raised. To ensure robustness and compatibility, the signature should be updated to accept and forward *args and **kwargs to both original_generate and generate_greedy_logprobs.
| def logged_generate(runner, prompts, max_tokens): | |
| if not collect_logits: | |
| outputs = original_generate(runner, prompts, max_tokens) | |
| records = [ | |
| {"prompt": prompt, "full_token_ids": ids, "full_text": text} | |
| for prompt, (ids, text) in zip(prompts, outputs, strict=True) | |
| ] | |
| else: | |
| scored_outputs = runner.generate_greedy_logprobs(prompts, max_tokens, 5) | |
| def logged_generate(runner, prompts, max_tokens, *args, **kwargs): | |
| if not collect_logits: | |
| outputs = original_generate(runner, prompts, max_tokens, *args, **kwargs) | |
| records = [ | |
| {"prompt": prompt, "full_token_ids": ids, "full_text": text} | |
| for prompt, (ids, text) in zip(prompts, outputs, strict=True) | |
| ] | |
| else: | |
| scored_outputs = runner.generate_greedy_logprobs(prompts, max_tokens, 5, *args, **kwargs) |
Align release graph capture with the persistent inputs updated by PCP runtime. Keep main on its upstream-selected buffer contract. Add a buffer-identity regression; numerical SFA accuracy validation remains pending in the draft PR. Signed-off-by: shenzhao <shenzhao9@huawei.com>
zhao-stack
changed the title
|
/e2e tests/e2e/pull_request/four_card/context_parallel/test_accuracy_v2.py::test_dsv3_2_sfa_pcp_model_runner_v2_graph_accuracy |
|
/e2e tests/e2e/pull_request/four_card/context_parallel/test_accuracy_v2.py::test_dsv3_2_sfa_pcp_graph_diagnostic[raw_logits-FULL_DECODE_ONLY] |
Keep graph mode and score observation fixed while changing only PCP size or the submitted prompt batch. Preserve the existing selected-prompt golden and original target assertions. Signed-off-by: shenzhao <shenzhao9@huawei.com>
|
/e2e tests/e2e/pull_request/four_card/context_parallel/test_accuracy_v2.py::test_dsv3_2_sfa_pcp_layout_diagnostic |
Keep only the release capture-buffer correction and its regression in the draft PR. Numerical validation still does not satisfy the unchanged v0.28.0 golden; retain the diagnostic records outside the production diff. Signed-off-by: shenzhao <shenzhao9@huawei.com>
zhao-stack
changed the title
|
/e2e tests/e2e/pull_request/four_card/context_parallel/test_accuracy_v2.py::test_dsv3_2_sfa_pcp_model_runner_v2_graph_accuracy |
Signed-off-by: shenzhao <shenzhao9@huawei.com>
|
/e2e tests/e2e/pull_request/four_card/context_parallel/test_accuracy_v2.py::test_dsv3_2_sfa_pcp_tensor_diagnostic |
Signed-off-by: shenzhao <shenzhao9@huawei.com>
|
/e2e tests/e2e/pull_request/four_card/context_parallel/test_accuracy_v2.py::test_dsv3_2_sfa_pcp_tensor_diagnostic |
Signed-off-by: shenzhao <shenzhao9@huawei.com>
|
/e2e tests/e2e/pull_request/four_card/context_parallel/test_accuracy_v2.py::test_dsv3_2_sfa_pcp_tensor_diagnostic |
|
/e2e tests/e2e/pull_request/four_card/context_parallel/test_accuracy_v2.py::test_dsv3_2_sfa_pcp_tensor_diagnostic |
1 participant
What this PR does / why we need it?
v0.28.0 sends global input buffers into MRV2 graph capture, whereas PCP runtime prepares persistent local input buffers. Main already selects PCP-local buffers upstream (#53515). This six-line candidate explicitly selects the PCP-local buffers on v0.28.0 before capture. It includes a regression for capture/runtime tensor identity and padding updates across PCP/non-PCP and release/main contracts.
This is a partial fix under investigation, not a resolved SFA PCP accuracy fix. It changes the observed release third-prompt output from
Rund compassestoRund596庄稼and makes all reported token/top-5-logit records match main in controlled diagnostic observations. It does not satisfy the unchanged release goldenRund959arkior make the complete original target pass. Keep this PR draft.Does this PR introduce any user-facing change?
Only the release PCP capture buffer source changes. Main retains its upstream-selected buffers. Investigation resumed: temporary tensor/metadata tracing is reintroduced on head
17e5b5467322b99a6263b7ce093960662e7933fc. These synchronizing test-only observers will be removed before any final fix is proposed. No golden, allowed-output, assertion, skip, upgrade-pointer or unrelated CPU collection-error changes. No changes to #16009, #15627, or baseline #16175; no labels or merge requested.How was this patch tested?
Isolated actual capture method with real CPU tensors and a recording backend: unpatched1failed/3passed, patched4passed. This isolates module initialization and NPU capture; it is not a full UT or numerical hardware test.
No-production-patch diagnostics34356957326: main graph passes; release graph fails at prompt3 (
compasses). Release eager cannot generate because of a separate missinginput_buffersattribute, so that is not an accuracy result.Original target with candidate34360230703: main1passed209.77s; release1failed208.26s at prompt2 (
ERIChiretailhallenging), aborting the comparison before prompt3.Candidate logits34362432893: main1passed, release1failed. All three token sequences and reported top-5 logits match between lanes. Relative to unpatched release, the first changed reported logit step is prompt3 generationstep4: token29490 (
596)14.1875 exceeds token20410 (compass)13.5625 and golden token32716 (959)12.9375.PCP/batch controls34364655334: with graph/scored observation fixed, both PCP1 and PCP2/single-third-prompt produce
Rund959arki; original PCP2/three-prompt batch producesRund596庄稼. The first reported logit difference appears at generationstep2, before the sampled-token difference atstep4. This happens in both versions. PCP1 also changes the resulting process/EP group topology and fails prompt2, so it is not a passing original-target baseline.These diagnostic/candidate runs used heads e53e/f780/8a09 rebased onto fixed Ascend
9e1cab90c0a8ec49369eb9aa1e8f387dbe5e4f1d. Empty baseline [CI] Diagnose SFA PCP accuracy on unchanged upstream main baseline #16175 uses033198/f5f691 and reports main3/3passed, release0/3passed at prompt2. It is not the same Ascend tree. Actual vLLM anchors are mainb2f685834a6456197e7033966fdef52a23f1abcdand release2cf0a6915ce544dc493a0990f2ea38d81601128a; main is not [Misc][Main2Main] Main2main 0909 #16009's a97dacb. Distinct runners and missing immutable model/tokenizer/image fingerprints preclude a strict environment-equivalence claim.Ruff/check/format, isolated assertion checks and git diff checks passed. Full
bash format.sh ciwas run; Windows/bin/bash, shellcheck/python3 availability and an earlier memory-allocation failure prevent full local verification. No hook configuration was weakened.Final head
fa440ba895f26ee6b658230fe018a42ed44030b9after removing diagnostics: main original target1passed207.34s; release original target1failed208.00s, prompt3 actualRund596庄稼versus expectedRund959arki. Actual checkoutfa440ba89 and rebasebase9e1cab90 verified in both logs. The full precision issue remains unresolved. Full-file validation has not been triggered because both original target lanes have not passed.Remaining blocker: no intermediate-tensor/operator trace identifies the remaining PCP2 multiple-request numerical discrepancy, and no matched-environment positive reference for the full release golden is established. Existing token/logit evidence does not justify further arithmetic changes or a root-cause-closure claim.
Continued investigation:
test_dsv3_2_sfa_pcp_tensor_diagnosticcompares original batch versus third-prompt-only with unchanged sampling/goldens. It records decode layer output hashes/norms and captured/runtime metadata values/pointers. CPU observer checks and Ruff passed; hardware neutrality and results are pending.Tensor diagnostic34431178636: original token behavior preserved in both lanes (batchthird596 / singlethird959), but layer snapshots remained warmup values and are NOT valid evidence of runtime equality. Actual request admission was staggered: third prefill mixed with earlier requests decoding; third firstdecode is workerstep4 in batch versusstep2 alone. Head
c583a3d126646b78d1a15f6fadc5f9c7d8990404corrects the temporary observer to record all compiled shapes, adds device update counts, extends collection to12 executions, omits mixed/prefill snapshot readouts and disables diagnostic compilecache reuse. No new production modification. Root cause remains open.Corrected observer34433161210: single-third output preserved and snapshot counters/hashes update, but both batch cases died before generation due to a diagnostic copy shape mismatch (4x7168 versus3x7168) in compiled execution. This is not numerical root-cause evidence. Head
a1a084e05cfb3cb46adaaee5b476fb327e84968dreplaces dynamic copy extents with a fixed-four-row index selection; production patch unchanged.vLLM main: vllm-project/vllm@b2f6858
Current blocker (2026-09-10): unchanged head
a1a084e05cfb3cb46adaaee5b476fb327e84968dcould not initialize the model in either run34434623808 or its single unchanged-head retry34435808925. Both lanes fail in ModelScopeModelFileSystemCache.save_model_metawithOSError: [Errno 122] Disk quota exceeded, before worker/model generation. Thus the latest observer shape correction has no NPU validation and there is still no valid paired layer trace. Automatic retries paused pending runner model-cache quota recovery. No original-target dual-lane pass, no full-file validation, and no numerical root-cause closure claimed. This PR remains a draft investigation with a partial capture-buffer correction and temporary diagnostics.