Conversation
|
👋 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. |
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 transitions the vLLM-Ascend platform from a simple environment-variable-based toggle for the V2 model runner to a more robust, whitelist-driven mechanism. By centralizing the enablement logic, the platform now intelligently selects the V2 runner for supported architectures and features while maintaining safe fallbacks to V1. This ensures consistent configuration behavior across distributed processes and improves maintainability by decoupling the enablement decision from upstream vLLM defaults. 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
|
![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:\n\nmarkdown\n[Platform][Feature] Implement Ascend-owned whitelist heuristics for Model Runner V2 enablement\n\n\nSuggested PR Summary:\n\nmarkdown\n### What this PR does / why we need it?\nThis PR introduces Ascend-owned whitelist heuristics for enabling the Model Runner V2 by default. Instead of relying purely on the 'VLLM_USE_V2_MODEL_RUNNER' environment variable, it now enables V2 by default for whitelisted architectures (currently 'Qwen3ForCausalLM') and supported speculative decoding methods ('eagle3', 'mtp', 'dflash'), provided Triton is available and the platform is not 310P. It also decouples and neutralizes the upstream V2 validation checks which do not apply to Ascend.\n\nFeedback: Two potential 'TypeError' issues were identified in 'vllm_ascend/mrv2_utils.py' where 'architectures' could be 'None' if explicitly configured as such. It is recommended to use 'or []' to guarantee an iterable list.\n\n### Does this PR introduce _any_ user-facing change?\nYes, Model Runner V2 is now enabled by default for Qwen3 models on supported Ascend platforms (non-310P) with Triton, whereas previously it required explicitly setting 'VLLM_USE_V2_MODEL_RUNNER=1'.\n\n### How was this patch tested?\nThe changes are covered by new unit tests in 'tests/ut/test_mrv2_utils.py' and 'tests/ut/patch/platform/test_patch_use_v2_model_runner.py', as well as updates to existing end-to-end tests.\n
| if getattr(model_config, "is_attention_free", False): | ||
| return False | ||
|
|
||
| architectures = getattr(model_config, "architectures", []) |
There was a problem hiding this comment.
If architectures is explicitly set to None in the model configuration, getattr(model_config, "architectures", []) will return None. This will cause a TypeError when iterating over it in the subsequent any(...) expression. Using or [] ensures that architectures is always an iterable list.
| architectures = getattr(model_config, "architectures", []) | |
| architectures = getattr(model_config, "architectures", []) or [] |
|
|
||
| if is_default_v2_model_runner_model(vllm_config): | ||
| if _v2_model_runner_environment_ready(vllm_config): | ||
| architectures = getattr(vllm_config.model_config, "architectures", []) |
There was a problem hiding this comment.
If architectures is explicitly set to None in the model configuration, getattr(vllm_config.model_config, "architectures", []) will return None. This will cause a TypeError when attempting to join the elements with ", ".join(architectures). Using or [] ensures that architectures is always an iterable list.
| architectures = getattr(vllm_config.model_config, "architectures", []) | |
| architectures = getattr(vllm_config.model_config, "architectures", []) or [] |
c27a1b5 to
4261b44
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 similar comment
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
4b32ac4 to
bc07dbe
Compare
bc07dbe to
0480dbd
Compare
7d8e38b to
5e04d59
Compare
ba90e6a to
fd9007a
Compare
a2e172f to
fd9007a
Compare
1525bae to
aa9b367
Compare
yjyang62
changed the title
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
d4fd328 to
da0c16d
Compare
Rebase vllm-project#11692 onto current vllm-project/vllm-ascend main. Replace the env-only use_v2_model_runner override with Ascend-owned whitelist heuristics (Qwen3ForCausalLM; eagle3/mtp/dflash; Triton; non-310P). Keep later unsupported-feature patches for spec-PP and Ascend-supported V1 features (dspark/dflash2). Explicit VLLM_USE_V2_MODEL_RUNNER still wins when set. Signed-off-by: yjyang62 <yangjinyang5@huawei.com>
Qwen3ForCausalLM now defaults to MRv2. The RLHF sleep/wake suite still depends on the V1 generate path after CuMem remap, so keep the subprocess server on VLLM_USE_V2_MODEL_RUNNER=0. Signed-off-by: yjyang62 <yjyang62@users.noreply.github.com> Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com>
GPU V2 stores capturing on the forward-context object. Ascend FIA treats _EXTRA_CTX.capturing as ACL graph capture. When Qwen3 defaults to V2 via the whitelist (env unset), extras leaked onto ctx.capturing and graph_task_group_begin ran on a non-capturing stream (error 107029). Route extras through additional_kwargs whenever VllmConfig.use_v2_model_runner is true, not only when VLLM_USE_V2_MODEL_RUNNER is set. Signed-off-by: yjyang62 <yjyang62@users.noreply.github.com> Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com>
Whitelist-enabled V2 routed extras through additional_kwargs whenever ctx.vllm_config.use_v2_model_runner was truthy. cpu-ut fixtures pass a bare MagicMock forward context, which auto-creates a truthy flag and hides capturing / max_tokens_across_dp. Require an actual bool True. Signed-off-by: yjyang62 <yjyang62@users.noreply.github.com> Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com>
GPU V2 sets forward_context.capturing during piecewise warmup. Ascend FIA treated that as ACL capture and called graph_task_group_begin, which failed with 107029 and hung Qwen3 whitelist-MRv2 e2e. Use the live NPU stream capture state (and skip PIECEWISE) instead of _EXTRA_CTX.capturing when wrapping FIA/PA kernels. Signed-off-by: yjyang62 <yjyang62@users.noreply.github.com> Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com>
cpu-ut stubs torch.npu as MagicMock, so is_current_stream_capturing() is truthy and FIA entered full_graph_fia. Require an actual True, matching use_v2_model_runner. Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com> Signed-off-by: yjyang62 <yjyang62@users.noreply.github.com>
A2 CI hung on LoRA-only generate and failed dflash acceptance when batch-size-based dynamic K (num_speculative_tokens_per_batch_size) was enabled under the Qwen3 default-V2 whitelist. Drop both from the default feature whitelist; static eagle3/mtp/dflash stay enabled, and VLLM_USE_V2_MODEL_RUNNER still overrides. Signed-off-by: yjyang62 <yjyang62@users.noreply.github.com> Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com>
GPU V2 PrefetchOffloader calls torch.cuda.is_current_stream_capturing during load_model. V1 already remaps that CUDA dummy to torch.npu; V2 torch_cuda_wrapper did not, so Qwen3 default-V2 prefetch e2e crashed on NPU. Signed-off-by: yjyang62 <yangjinyang5@huawei.com>
Qwen3ForCausalLM now defaults to Model Runner V2. GPU V2 prefetch plus NZ graph capture matches the eager baseline, so the strict xfail on test_prefetch_offload_accuracy[NZ-graph] XPASS-fails a2-1 CI. Keep the V1 AscendPrefetchOffloader fail-fast for the unsupported combo. Signed-off-by: yjyang62 <yangjinyang5@huawei.com>
Route _EXTRA_CTX through additional_kwargs when use_v2_model_runner(get_current_vllm_config()) is true. Whitelist-default V2 leaves VLLM_USE_V2_MODEL_RUNNER unset and GPU ForwardContext has no vllm_config, so env/ctx checks leaked GPU capturing onto FIA. Restore FIA/PA graph_task_group gating to _EXTRA_CTX.capturing. Signed-off-by: yjyang62 <yangjinyang5@huawei.com>
Replace `_extra_ctx_uses_additional_kwargs` with `if use_v2_model_runner(get_current_vllm_config()) is True`. Guard xlite `index_full_mask` for mypy after merging main. Signed-off-by: yjyang62 <yangjinyang5@huawei.com>
Keep getattr/setattr the same as before, only replacing the helper with `if use_v2_model_runner(get_current_vllm_config()) is True`. Signed-off-by: yjyang62 <yangjinyang5@huawei.com>
Keep getattr/setattr the same as main. Only replace the V2 condition with `use_v2_model_runner(get_current_vllm_config()) is True`. Signed-off-by: yjyang62 <yangjinyang5@huawei.com>
da0c16d to
3538c10
Compare
get_current_vllm_config() raises in cpu-ut attention fixtures. Fall back to V1 extra-ctx attrs so FIA can read capturing. Signed-off-by: yjyang62 <yangjinyang5@huawei.com> Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com>
Compiled FIA/MoE read _EXTRA_CTX, which called use_v2_model_runner()
and hit logger.warning_once / info_once. Dynamo cannot trace those
logs, so LoRA and non-whitelist models failed compile, and V2 MoE
baked additional_kwargs.get("moe_comm_method") as None.
Disable Dynamo on the extras helper, proxy getattr/setattr, and
use_v2_model_runner so isolation still uses get_current_vllm_config
at runtime.
Signed-off-by: yjyang62 <yangjinyang5@huawei.com>
Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com>
Decorating __getattr__/__setattr__ with torch._dynamo.disable made mypy treat _EXTRA_CTX as having no dynamic attributes. Keep the dunders undecorated and disable the helpers they call instead. Signed-off-by: yjyang62 <yangjinyang5@huawei.com> Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com>
cpu-ut installs torch 2.10, which marks @torch._dynamo.disable with _torchdynamo_disable instead of _dynamo_disable. Signed-off-by: yjyang62 <yangjinyang5@huawei.com> Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com>
…park (#16626) ### What this PR does / why we need it? Stacked on #16203. #16203 currently defaults Model Runner V2 only for `Qwen3ForCausalLM` plus static `eagle3` / `mtp` / `dflash`. This PR only extends those two whitelists so more architectures and `dspark` can default to V2. Gating logic is otherwise unchanged: LoRA and dynamic speculative decoding (`num_speculative_tokens_per_batch_size`) still stay on V1, and `VLLM_USE_V2_MODEL_RUNNER` still overrides the whitelist. Default-V2 model architectures: - `Qwen3ForCausalLM` - `Qwen3MoeForCausalLM` - `MiniMaxM2ForCausalLM` - `DeepseekV3ForCausalLM` - `DeepseekV32ForCausalLM` - `GlmMoeDsaForCausalLM` - `DeepseekV4ForCausalLM` Default-V2 speculative methods: `eagle3` / `mtp` / `dflash` / `dspark`. ### Does this PR introduce _any_ user-facing change? Yes. The architectures above now default to Model Runner V2 when Triton is present, the platform is not 310P, and the feature whitelist is satisfied. Other models still default to V1. Explicit `VLLM_USE_V2_MODEL_RUNNER=0/1` is unchanged. ### How was this patch tested? - Unit tests updated: `tests/ut/test_mrv2_utils.py` (architecture parametrize, `dspark` on the feature whitelist) - CI: cpu-ut / e2e on this PR. - vLLM main: vllm-project/vllm@84030bb --------- Signed-off-by: yjyang62 <yangjinyang5@huawei.com> Signed-off-by: yjyang62 <yjyang62@users.noreply.github.com> Signed-off-by: Spicy-Stick <873805887@qq.com> Signed-off-by: ZhangwenTaoHW <zhangwentao101@huawei.com> Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com> Co-authored-by: Spicy-Stick <873805887@qq.com> Co-authored-by: ZhangwenTaoHW <zhangwentao101@huawei.com> Co-authored-by: GDzhu01 <116337067+GDzhu01@users.noreply.github.com>
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
|
||
| idx = 0 | ||
| index_mask = self.xlite_config.index_full_mask or [True] * self.xlite_config.n_layers | ||
| index_mask = getattr(self.xlite_config, "index_full_mask", None) or [True] * self.xlite_config.n_layers |
There was a problem hiding this comment.
This seems unnecessary. Update xlite with pip install xlite==0.2.0rc1 and the lint issue should be gone.
2 participants
What this PR does / why we need it?
Rebase #11692 onto current
main.maincurrently overridesVllmConfig.use_v2_model_runnerwith an env-only gate: unlessVLLM_USE_V2_MODEL_RUNNERis set, Ascend always uses Model Runner V1. Upstream vLLM instead defaults V2 from its own architecture / Triton / feature checks. Following those GPU defaults on NPU can crash, because the Ascend V2 runner does not yet cover the same model and feature set.This PR replaces that env-only property with an Ascend-owned default in
vllm_ascend/mrv2_utils.py, installed whereverVllmConfigis created or unpickled (frontendNPUPlatform.check_and_update_config, EngineCore viavllm_ascend/__init__.py, and worker init).Default V2 when all of the following hold; otherwise V1:
Qwen3ForCausalLM(generate, not hybrid, not attention-free).eagle3/mtp/dflash.VLLM_USE_V2_MODEL_RUNNER=0/1still wins over the whitelist.Related changes in the same patch file:
_validate_v2_model_runner. GPU V2 feature/Triton checks must not reject an Ascend enablement decision (including an explicit env override).prefill context parallelismon v0.28.0, and honorresolve_spec_pp_supportfor spec+PP.dspark speculative decodinganddflash2 drafts, but install that patch withhasattrinstead ofvllm_version_is("0.28.0")so SHA /__version__="dev"CI checkouts do not patch a missing API.Does this PR introduce any user-facing change?
Yes.
vllm serveofQwen3ForCausalLM(for example Qwen3 dense) now defaults to Model Runner V2 on A2/A3/A5 when Triton is present and speculative decoding is absent oreagle3/mtp/dflash. Models not on the architecture whitelist (DeepSeek, GLM, Qwen3-MoE, hybrid models, 310P) still default to V1. ExplicitVLLM_USE_V2_MODEL_RUNNERis unchanged.How was this patch tested?
Unit tests added/updated:
tests/ut/test_mrv2_utils.py(architecture / feature / env / 310P / Triton gates, validation no-op)tests/ut/patch/platform/test_patch_use_v2_model_runner.py(property wiring, V1 feature exceptions, v0.28.0 PCP exception)tests/e2e/pull_request/one_card/test_attention_v1_precision.py: mock TP/DP groups because Qwen3 now defaults to V2 andNPUPlatform.set_additional_forward_contextreads those groups.CI: cpu-ut / e2e on this PR.
vLLM main: vllm-project/vllm@84030bb