[Graph][Fusion] Integrating inductor pass and npugraph ex pass - #6354
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. |
There was a problem hiding this comment.
Code Review
This pull request refactors the graph fusion passes by introducing a BasePattern class to unify pattern registration for both the inductor and npugraph_ex backends. This is a good architectural improvement. However, the current implementation contains two critical bugs that will cause runtime errors: the new BasePattern calls a method that is not defined, and a class is removed without updating its usage elsewhere. My review includes specific comments and suggestions to address these critical issues.
I am having trouble creating individual review comments. Click here to see my feedback.
vllm_ascend/compilation/passes/base_pattern.py (27-43)
The call to self.get_extra_check() on line 31 will raise an AttributeError at runtime as the method is not defined in BasePattern. Additionally, the extra_check variable is unused. To fix the runtime error and improve consistency, I suggest removing the call to the undefined method and applying the existing extra_stream_scope_check to both pm.register_replacement and torchair.register_replacement.
def register(self, pm_pass: PatternMatcherPass) -> None:
pattern_fn = self.get_pattern()
replacement_fn = self.get_replacement()
example_inputs = self.get_example_inputs()
pm.register_replacement(
pattern_fn, replacement_fn, example_inputs,
pm.fwd_only, pm_pass, extra_check=extra_stream_scope_check
)
torchair.register_replacement(
search_fn=pattern_fn,
replace_fn=replacement_fn,
example_inputs=example_inputs,
extra_check=extra_stream_scope_check,
)vllm_ascend/compilation/npugraph_ex_passes/graphex_norm_quant_fusion_pass.py (28-87)
Removing the GraphEXAddRMSNormQuantPattern class introduces a NameError at runtime because GraphEXAddRMSNormFusionPass in this same file still attempts to use it on line 242.
Since AddRMSNormQuantPattern (which now uses BasePattern) handles registration for both torchair and inductor, the call to GraphEXAddRMSNormQuantPattern(...).register() is likely no longer needed and should be removed from GraphEXAddRMSNormFusionPass.__init__.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
b2c0799 to
72d641d
Compare
|
please help to verify npugraph ex is available @ChenCangtao |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
88a3ab5 to
e272124
Compare
78ec79c to
2a9a175
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: wxsIcey <1790571317@qq.com>
2a9a175 to
0764de0
Compare
…ascend into qwen3next_rebase * 'qwen3next_rebase' of https://github.com/845473182/vllm-ascend: [Bugfix][DispatchFFNCombine] resolve vec error caused by unaligned UB access (vllm-project#6707) [Lint] Adapt lint tools for windows (vllm-project#6727) [main][Docs] Fix typos across documentation (vllm-project#6728) [Feat.][310P]: weightNZ feature with quant or unquant. (vllm-project#6705) [Feat.][310P] addrmsnorm for 300I DUO (vllm-project#6704) [Graph][Fusion] Integrating inductor pass and npugraph ex pass (vllm-project#6354) [bugfix] adapt bugfix for norm_quant_fusion_pass to npugraph_ex (vllm-project#6726) [doc] add A2 series doc for GLM5.md (vllm-project#6717)
…project#6354) ### What this PR does / why we need it? Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com>
…project#6354) ### What this PR does / why we need it? Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com>
…project#6354) ### What this PR does / why we need it? Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com>
…project#6354) Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 N/A all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com>
…project#6354) Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 N/A all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com> [bugfix] Pass adaptation mulsadd trion pass adaptation for 0.13.0
…project#6354) Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 N/A all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com> [bugfix] Pass adaptation mulsadd trion pass adaptation for 0.13.0
…project#6354) ### What this PR does / why we need it? Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com>
…project#6354) Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 N/A all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com> [bugfix] Pass adaptation mulsadd trion pass adaptation for 0.13.0
…project#6354) ### What this PR does / why we need it? Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com>
…project#6354) ### What this PR does / why we need it? Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com> Signed-off-by: nanxing <1014662416@qq.com>
…project#6354) ### What this PR does / why we need it? Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com>
…project#6354) ### What this PR does / why we need it? Integrating inductor pass and npugraph ex pass, see RFC: vllm-project#6347 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? all tests passed. - vLLM version: v0.14.1 - vLLM main: vllm-project/vllm@dc917cc --------- Signed-off-by: wxsIcey <1790571317@qq.com>
What this PR does / why we need it?
Integrating inductor pass and npugraph ex pass, see RFC: #6347
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
all tests passed.