Repository navigation
[Bugfix] Preserve CUDA toolkit header precedence in a2a JIT builds - #7599
Conversation
Signed-off-by: 武垚乐 <wuyaole@mininglamp.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This PR appears to belong to: docs/design/module/diffusion/parallelism.md, docs/design/module/diffusion/index.md. Module owners: @princepride @Bounty-hunter @xuechendi Routing: @princepride via module of the changed files, semantic router, CODEOWNERS; @Bounty-hunter via module of the changed files, semantic router; @xuechendi via module of the changed files, semantic router @LeslieWylie, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
|
Author self-review for 72bff86: checked the generated host/CUDA include ordering, toolkit headers taking precedence over mismatched wheel headers, and retention of wheel-only cuSPARSE/NCCL headers and NCCL library discovery. The focused a2a tests and Ruff passed. On H20 (nvcc 13.0.88, PyTorch 2.13.0+cu130, wheel compiler/CRT headers 13.4.59), the toolkit-first |
|
@princepride Could you review this focused CUDA header-precedence fix? Author self-review and the real extension build/load reproduction are documented. Build, pre-commit and DCO pass; docs are pending. If the approach looks good, could you help get it approved and merged after required checks complete? Thanks! |
hsliuustc0106
added
bug
Omni ReviewBot: no human activity for 7 days@LeslieWylie this pull request has had no human commit, comment or review since 2026-09-16. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
|
Still actively maintained. Current build, pre-commit, DCO and docs checks are all green, and the branch has no merge conflict. The mixed CUDA-header reproduction and actual extension build/load evidence remain in the author self-review. Next step is maintainer review/approval of this focused fix; no new code changes are needed to respond to the inactivity reminder. Please keep the PR open. |
Omni ReviewBot: no human activity for 7 days@LeslieWylie this pull request has had no human commit, comment or review since 2026-09-24. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Signed-off-by: 武垚乐 <wuyaole@mininglamp.com>
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
|
@princepride Could you prioritize a focused review of this small a2a JIT build fix in the next review cycle? The current main path still supplies NVIDIA wheel headers through I rechecked head The review question is whether the later system includes preserve toolkit precedence without breaking wheel-only installations. If this is acceptable, could you approve and help move it toward merge? Otherwise, please identify the remaining blocker or the right reviewer so I can address it on this branch. Thank you. |
|
I will review it tmr |
vllm-omni-review-bot
left a comment
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
2 actionable finding(s).
CI at
254284485371(2026-10-10T10:05:34.471617+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 2 finding(s) below |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [claim-verified] parametrize covers toolkit with and without cusparse.h (test:35-36, :65-66).
- [claim-verified] toolkit before extra flags: pytorch cpp_extension _write_ninja_file_to_build_library sets common_cflags += '-isystem CUDA_HOME/include' then cuda_flags = common_cflags + ... + extra_cuda_cflags.
- [claim-verified] extra_include_paths become -I user includes before -isystem system includes (same function, user_includes then system_includes) — that is the bug this PR removes.
- [validated] 1 correctness: omitting extra_include_paths and appending vendor -isystem on extra_cflags and extra_cuda_cflags (a2a_permute.py:81-86) matches the stated mixed-header fix; nccl lib/header raises unchanged.
- [validated] 2 simplifiability: no dead production branch; toolkit_has_cusparse is test-only and does not affect _ensure_built.
- [validated] 3 breaking/API: no public signature or default change. ensure_a2a_permute_available / ulysses_qkv_fwd / ulysses_o_rev unchanged. Old extra_include_paths was a private load() kwarg; in-repo grep found no stale extra_include_paths consumers outside this test (updated).
2 actionable finding(s).
Verdict: COMMENT
See inline comments below.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
| vendor_paths.append(args[1]) | ||
| assert set(vendor_paths) == {str(cu13_include), str(nccl_include)} | ||
| # cpp_extension places its toolkit system include before extra flags. | ||
| paths = [str(toolkit_include), *vendor_paths] |
There was a problem hiding this comment.
[P2] This test never observes toolkit-vs-vendor header order
Evidence and suggested fix
This test never observes toolkit-vs-vendor header order. load() is monkeypatched to load_kwargs.update(kwargs) and is not given toolkit_include; paths = [str(toolkit_include), *vendor_paths] then walks files the test itself created, so both toolkit_has_cusparse arms pass by construction. The CPU marker also cannot compile the mixed-header case. Keep assert not load_kwargs.get("extra_include_paths"), the vendor -isystem set, and extra_ldflags; drop the parametrize, unused toolkit fixture, and selected_header.
Evidence: Trigger: tests/diffusion/distributed/test_a2a_permute.py::test_jit_build_prefers_toolkit_and_keeps_wheel_headers on this PR head. Adverse effect / unmet requirement: the test is named and commented as proving cpp_extension toolkit-precedence, but that order is hardcoded and cannot fail, so a real toolkit-vs-wheel header mix would not be caught. tests/diffusion/distributed/test_a2a_permute.py:16 pytestmark = [pytest.mark.core_model, pytest.mark.cpu]. :40-43 monkeypatch.setattr(torch.utils.cpp_extension, "load", lambda **kwargs: load_kwargs.update(kwargs),) — never compiles and never receives toolkit_include. :31-36 creates local toolkit_include headers; production glob is nvidia/*/include, so that dir is not a load() input. :50 assert not load_kwargs.get("extra_include_paths") and :52-57 already pin the real contract (vendor -isystem on extra_cuda_cflags/extra_cflags). :58-59 # cpp_extension places its toolkit system include before extra flags. then paths = [str(toolkit_include), *vendor_paths]. :61-66 selected_header walks that constructed list over files the test touched, so both parametrize arms pass by construction.
Suggestion: assert load_kwargs["extra_ldflags"] == [str(nccl_lib)]
|
|
||
| def test_jit_build_includes_cuda_headers_from_nvidia_wheels(tmp_path, monkeypatch) -> None: | ||
| @pytest.mark.parametrize("toolkit_has_cusparse", [False, True]) | ||
| def test_jit_build_prefers_toolkit_and_keeps_wheel_headers(tmp_path, monkeypatch, toolkit_has_cusparse) -> None: |
There was a problem hiding this comment.
[P3] This CPU stub never observes toolkit-vs-wheel header precedence: load() is monk…
Evidence and suggested fix
This CPU stub never observes toolkit-vs-wheel header precedence: load() is monkeypatched to capture kwargs only, toolkit_include is prepended locally rather than passed into load(), pytestmark remains core_model+cpu, and a2a_permute.cu does not include cusparse.h. Rename test_jit_build_prefers_toolkit_and_keeps_wheel_headers to a flag-shape name such as test_jit_build_uses_vendor_isystem_not_extra_include_paths.
Evidence: Trigger: collecting/running this parametrized CPU node. Adverse effect: the node id claims toolkit header precedence this file cannot observe; a green result can be read as compiler-order proof. tests/diffusion/distributed/test_a2a_permute.py:16 pytestmark = [pytest.mark.core_model, pytest.mark.cpu]; :20 def test_jit_build_prefers_toolkit_and_keeps_wheel_headers(tmp_path, monkeypatch, toolkit_has_cusparse) -> None:; :40-43 monkeypatch.setattr(torch.utils.cpp_extension, "load", lambda **kwargs: load_kwargs.update(kwargs),) — load kwargs never include toolkit_include; :59 paths = [str(toolkit_include), *vendor_paths] synthesizes toolkit-first search in the test. Unchanged by this diff, present in the PR-time tree: vllm_omni/diffusion/distributed/csrc/a2a_permute.cu:9-19 include CUDAGuard/NCCL/torch headers and do not include cusparse.h.
Suggestion: def test_jit_build_uses_vendor_isystem_not_extra_include_paths(tmp_path, monkeypatch) -> None:
…llm-project#7599) Signed-off-by: 武垚乐 <wuyaole@mininglamp.com> Co-authored-by: 武垚乐 <wuyaole@mininglamp.com>
Problem
a2a_permuteadds every NVIDIA wheel include directory throughextra_include_paths. PyTorch renders those as-I, while it supplies the selected CUDA toolkit using-isystem. Wheel CUDA headers therefore take precedence over the headers paired with the selected nvcc. On a mixed installation this fails in generated launch stubs with an incompatible__cudaLaunchmacro.Adding the toolkit to the front of
extra_include_pathsis insufficient: the compiler treats a directory also present as-isystemas a system directory, leaving the wheel-Ientries ahead of it.Change
Append NVIDIA wheel directories as system include flags, after PyTorch's toolkit defaults, for both CUDA and host compilation. Keep the wheel paths for cuSPARSE/NCCL headers missing from toolkit-only locations, retain NCCL library discovery, and quote paths. No CUDA_HOME environment override or blanket removal of the cu13 package is required.
Validation
-Iapproach reproduced the__cudaLaunchcompilation error. The final system-include change compiled and loaded the actual a2a CUDA extension successfully.The earlier actual CUDA build/load evidence above remains historical.
The October 3 run rechecks current-main JIT flag generation and workspace
behavior on CPU; it does not claim a new CUDA build or distributed run.
October 3 refresh
Merged upstream main
ee8fdab1without rewriting the PR branch history.Validation used a fresh complete source package for that merged revision in an
isolated Linux environment: Python 3.12, vLLM 0.29.0, PyTorch 2.13.0+cu130,
transformers 5.14.1 and PyAV 18.1.0. This is not a vLLM 0.30 compatibility claim.
CPU checks hide all GPUs. The LoRA suites disable vLLM's host pin-memory flag
inside the test process because the installed CUDA wheel otherwise tries to pin
with no visible device. No serving process or installed package was modified.
Changed-file pre-commit gates passed, including mypy-3.10, SPDX, test marks,
forbidden imports, CUDA-call ratchet, formatting and applicable Markdown checks.
The diff-scoped self-review checked lifecycle/ownership, default behavior,
examples policy, error paths and the evidence behind performance claims.
AI assistance: Codex prepared the merge/fixes, source review, test execution and
this PR update. The recorded validation and remaining gaps below are the scope
of that verification; maintainer review is still required.
Focused result: 3 passed.
Test selection (run in the CPU-only harness described above):