Skip to content

Fix pure-text AFD model config identity - #174

Merged
jiangkuaixue123 merged 2 commits into
vllm-project:mainfrom
ShwStone:fix/issue-172-model-config-identity
Jul 29, 2026
Merged

Fix pure-text AFD model config identity#174
jiangkuaixue123 merged 2 commits into
vllm-project:mainfrom
ShwStone:fix/issue-172-model-config-identity

Conversation

@ShwStone

Copy link
Copy Markdown
Contributor

Purpose

Fix the model configuration regression introduced by #144 and reported in
#172.

get_afd_model_config() copied and replaced hf_config without updating
hf_text_config. For pure-text models, this broke the original identity:

model_config.hf_text_config is model_config.hf_config

vLLM-Ascend consequently treated DeepSeekV2-Lite as a model with a distinct
nested text configuration. Under Attention TP2 with FlashComm1/SP, MLA skipped
the required query/KV gather and passed TP-local x together with full-batch
RoPE cos/sin, causing npu_interleave_rope to fail.

The regression was confirmed across #144:

  • 220c7626de5195bdcf24271659f0c1519a71f2a3: passes
  • 355a1cda4a1f170b0fd5db0029eba8c521151241: fails

Issue

Scope

  • In scope:
    • Preserve hf_config/hf_text_config identity when converting a pure-text
      model config to its AFD architecture.
    • Preserve a genuinely distinct nested hf_text_config unchanged.
    • Add unit coverage for both configuration layouts.
  • Out of scope:
    • Changes to vLLM or vLLM-Ascend.
    • Changes to FlashComm1/SP scheduling or MLA implementation.
    • Changes to Async CAM communication or MoE ubatching.
    • Changes to multimodal model configuration semantics.

Implementation Notes

get_afd_model_config() now creates one private hf_config copy, updates its
architecture, and assigns it to the copied ModelConfig.

When the source is a pure-text configuration:

model_config.hf_text_config is model_config.hf_config

the copied model config points both fields to the new private hf_config.
When the source has a genuinely distinct nested hf_text_config, that object
remains distinct and unchanged.

This is a plugin-owned model configuration fix. It does not patch or modify
vLLM/vLLM-Ascend source code.

Test Plan

Focused unit coverage:

python -m pytest -vv \
  tests/unit/package/test_package.py \
  -k "afd_model_config"

Ascend NPU Async CAM regression coverage:

ASCEND_RT_VISIBLE_DEVICES=0,1,2,3 \
AFD_NPU_E2E_MODEL=/path/to/DeepSeek-V2-Lite \
AFD_NPU_ASYNC_CAM_E2E_DEVICES=0,1,2,3 \
AFD_NPU_E2E_VLLM_BIN=vllm \
AFD_NPU_E2E_STARTUP_TIMEOUT=900 \
python -m pytest -vv -s \
  tests/e2e/models/deepseek_v2_lite/test_async_cam_npu.py::test_deepseek_v2_lite_async_cam_attn_dp1tp2_ffn_dp2ep2_smoke

Test Result

Both targeted tests passed:

  • Focused afd_model_config unit tests: passed
  • Async CAM Attention DP1TP2 + FFN DP2EP2 NPU E2E smoke test: passed

The E2E result confirms that the existing main-branch Async CAM test can run
with Attention TP2 and FlashComm1 without the previous RoPE batch-dimension
failure.

Docs Impact


Essential PR Checklist
  • Purpose is clear and linked to public context when possible.
  • Scope is bounded.
  • Compatibility with vLLM v0.19.1 is considered.
  • No changes are made to the vLLM source checkout.
  • Plugin-owned classes or explicit dotted class paths are preferred over monkey patches.
  • Any compat shim or monkey patch is isolated, idempotent, version-guarded, documented, and tested.
  • Imports remain CPU-safe; CUDA-heavy work is delayed or GPU-gated.
  • Validation evidence is included, including skipped GPU tests when applicable.
  • Documentation impact is stated.

Copilot AI review requested due to automatic review settings July 29, 2026 05:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a regression in get_afd_model_config() where converting a pure-text ModelConfig to an AFD architecture broke the invariant hf_text_config is hf_config, which downstream vLLM-Ascend logic relies on to distinguish pure-text vs nested-text/multimodal configurations.

Changes:

  • Update get_afd_model_config() to create a single private hf_config copy and (when applicable) re-point hf_text_config to preserve pure-text object identity.
  • Add unit tests covering both layouts: pure-text identity preservation and nested hf_text_config preservation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
afd_plugin/model_executor/models/model_utils.py Preserves hf_config/hf_text_config identity for pure-text configs while keeping truly distinct nested text configs unchanged.
tests/unit/package/test_package.py Adds targeted unit coverage for both configuration layouts to prevent regressions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ShwStone
ShwStone force-pushed the fix/issue-172-model-config-identity branch from 52233c2 to 3c99fdb Compare July 29, 2026 05:18
@jiangkuaixue123

Copy link
Copy Markdown
Collaborator
image Fix DCO

Signed-off-by: ShwStone <haowenshi@outlook.com>

@jiangkuaixue123 jiangkuaixue123 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be safer to use deepcopy(model_config) here and then update afd_model_config.hf_config.architectures? deepcopy preserves aliasing within the copied object graph, so the pure-text invariant hf_text_config is hf_config would be retained automatically, and it would reduce the risk of missing other identity relationships if ModelConfig evolves.

The semantic difference is that a genuinely distinct/nested hf_text_config would also become a private copy, whereas this patch intentionally keeps it shared with the source config. Is that sharing required? If not, deepcopy may provide stronger isolation and a simpler implementation. Otherwise, could we document why only hf_config should be copied so this choice is explicit?

@ShwStone
ShwStone force-pushed the fix/issue-172-model-config-identity branch from 3c99fdb to a6e6bbc Compare July 29, 2026 06:13
@ShwStone

Copy link
Copy Markdown
Contributor Author

I confused GPG signing with DCO; I always thought signing was enough to pass DCO. I've already recommitted.

Signed-off-by: ShwStone <haowenshi@outlook.com>
@ShwStone

Copy link
Copy Markdown
Contributor Author

@jiangkuaixue123 Agreed — the deepcopy approach is cleaner and preserves the aliasing invariants automatically. I've switched the implementation to deepcopy(model_config) in 38df180 and validated it on real hardware (DeepSeekV2-Lite, Attention TP2 with FlashComm1/SP): the npu_interleave_rope failure is gone and the model runs correctly. Tests updated accordingly. PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: PR #144 breaks pure-text model config identity and causes Async CAM TP2 FlashComm1 RoPE failure

3 participants