[TRTLLM-14558][chore] Add the forwarding modules for the retired Attention paths - #17969
Conversation
559253c to
094cc4a
Compare
…n paths Second half of the Attention relocation. PR NVIDIA#17968, merged as a56ec20, moved the Attention module layer to tensorrt_llm/_torch/attention/ and the attention backends to tensorrt_llm/_torch/attention/backends/; this change puts definition-free forwarding modules back at the two retired paths that evidence shows callers outside this repository still use, so they keep working through the deprecation window. tensorrt_llm/_torch/modules/attention.py forwards Attention tensorrt_llm/_torch/attention_backend/__init__ forwards the old package root's whole __all__ Neither path is a guess. Before the move, both were named by imports in examples/llm-api/out_of_tree_example/modeling_opt.py -- lines 9 and 15 of the file the "adding a new model" walkthrough tells users to copy into their own tree. NVIDIA#17968 repointed that example and the walkthrough in the same change, so nothing in this repository is broken today; what that change cannot reach is every copy users already made from the earlier text. Those copies are what these modules serve. A forwarding module for one path and a hard cut for the other would leave such a copy broken anyway, so the two paths are treated the same way. Every other retired coordinate in the move is a hard cut: private, and clean on every public-surface criterion that can be scanned in-tree. The two export sets differ because the two retired paths are different kinds of thing. modules/attention.py is a module, and the only name any evidence source shows reaching through it is Attention; its other public names appear in no __all__ anywhere and are not forwarded. attention_backend/__init__.py is a package root, and a package root's __all__ IS its declared public surface, so this shim reproduces that list exactly -- including the two names the original adds under `if IS_FLASHINFER_AVAILABLE`, behind the same condition. Forwarding only the one name the example happens to use would give a caller who imported any of the other ten an ImportError with no migration hint, from a module that had just imported successfully and warned -- strictly worse than the clean ModuleNotFoundError a hard cut would have produced. That list is re-derived from the canonical __all__ rather than copied from an older revision: NVIDIA#18025 removed star attention after these modules were first written, so StarAttention and StarAttentionMetadata are not forwarded and the star_flashinfer import is gone. Keeping them would have made the shim raise ModuleNotFoundError on precisely the FlashInfer-enabled configuration it exists to serve, taking the other eleven names down with it. Four checks in tests/unittest/_torch/attention/test_backends_importable.py keep both modules honest: each retired path hands back the canonical object itself rather than a copy, the package shim re-exports every name the canonical package does, its __all__ matches the canonical __all__, and both paths warn with FutureWarning when imported. The __all__ check is the one that earns its keep -- it is what would have caught the star attention drift above, which on today's main is a ModuleNotFoundError raised only for a caller who has FlashInfer installed, the one configuration those two names ever served. It compares sets, not ordered lists: __all__ order binds nothing at import time, so freezing it would fail CI on a cosmetic reorder of the canonical list while catching no drift a caller could observe. The warning check drops the module from sys.modules and puts it back, because a module-level warnings.warn fires only on the first import in a process and an earlier test in the same one may already have spent it. The file is already collected by the directory-level unittest/_torch/attention test-list entries, so no test list changes. Custom-op registrations are unaffected: importing either file imports the canonical module, so its registration side effect runs as before. Both re-export the object rather than copying it -- `old.X is canonical.X` -- so isinstance and existing pickles still work. Both warn with FutureWarning, not DeprecationWarning: the latter is on Python's stock ignore list outside `__main__` and would never reach the callers these modules exist for. No in-tree caller routes through either of them: a scan of the tree for both retired paths reports only the forwarding modules themselves, the guard test added here that imports them on purpose, and two blog permalinks pinned to a historical commit that must stay as they are. CODEOWNERS does carry a COMPATIBILITY FORWARDING MODULES block for this Epic, and its rule is that each such module keeps the owner its pre-move path had; both paths were NVIDIA/trt-llm-torch-attention-devs before the move, and with these files present they resolve instead to NVIDIA/trt-llm-models-devs and NVIDIA/trt-llm-runtime-devs -- two different teams, neither the pre-move one. The two lines that would satisfy the block are held out of this change because /.github/CODEOWNERS is itself owned by NVIDIA/trt-llm-infra-devs and NVIDIA/trt-llm-oss-compliance, so adding them pulls two more required approvals onto a four-file PR; whether that trade is worth making is a question for review rather than something this commit should settle. Either way, add the Attention team as a reviewer by hand. This change also carries the copyright header for the relocated attention/backends/__init__.py. The header belongs on that file, but adding it during the relocation itself would have cut git's rename similarity for that short module to a few points above the detection threshold. Holding it back worked: the rename is recorded in a56ec20 at R092, so `git blame` for those lines is anchored in history already and the header can land without risking it. The forwarding modules were kept out of NVIDIA#17968 because git pairs renames from adds and deletes, and a squash merge collapses any in-PR split. Landing them together with the move would have made git see new files plus rewritten old ones, and `git blame` for the moved lines would have pointed at the relocation instead of at their authors. Signed-off-by: Yihui Lu <269394165+YihuiLu512@users.noreply.github.com>
094cc4a to
056b7a3
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughAdded compatibility shims for retired attention import paths. The shims re-export canonical objects, emit ChangesAttention import compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change restores retired Attention import paths by forwarding them to the canonical implementations and warning callers to migrate. The compatibility behavior is covered without an identified current-head merge risk. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant LegacyImport
participant AttentionShim
participant CanonicalAttention
LegacyImport->>AttentionShim: import retired attention path
AttentionShim-->>LegacyImport: emit FutureWarning
AttentionShim->>CanonicalAttention: resolve canonical exports
CanonicalAttention-->>LegacyImport: provide attention objects
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/bot run --disable-fail-fast |
|
PR_Github #71890 [ run ] triggered by Bot. Commit: |
|
PR_Github #71890 [ run ] completed with state |
Description
Second half of the Attention consolidation — Epic TRTLLM-14558, ticket TRTLLM-14841 (the PR title carries the Epic key, the commit subject carries the ticket key). #17968 retired two import paths; this PR puts a definition-free forwarding module back at each of them so callers outside this repository keep working through the deprecation window, and lands the NVIDIA copyright header that #17968 had to hold back.
tensorrt_llm/_torch/modules/attention.py_torch/attention/attention.py; re-exportsAttentiontensorrt_llm/_torch/attention_backend/__init__.py_torch/attention/backends/; re-exports all 11 names of the canonical__all__tensorrt_llm/_torch/attention/backends/__init__.pytests/unittest/_torch/attention/test_backends_importable.py__all__, 1 warninggit diff --statagainst the branch point3a3871d8b1: 4 files, +193 / −0, a single commit.Both forwarding modules define nothing — a header, a docstring, an import list, one
warnings.warn, one__all__, and in the package one a conditional FlashInfer import block with an__all__ +=. They re-export the canonical objects rather than copies (old.X is canonical.X), soisinstanceand pre-migration pickles keep working; importing either one imports the canonical module, so custom-op registration side effects run exactly as before. Both warn withFutureWarning, matching 7 of the 9 compatibility shims already in the tree.Still Draft on purpose. No Jenkins pipeline has ever run on this branch. The blocking dependency is discharged, but the question below — whether out-of-repo evidence warrants a shim at all — is worth settling before spending pipeline time. Remaining steps, in order: settle the CODEOWNERS question, flip out of Draft,
/bot run --disable-fail-fast. @NVIDIA/trt-llm-torch-attention-devs is already a requested reviewer. TheVisualGenlabel was applied at PR creation, to a head that still touched_torch/visual_gen/; after the rebase onto these four files it no longer applies — replaying.github/scripts/label_component.pyover them yields no component label at all.Three claims the earlier body made are no longer true. It said 13 forwarded names: #18025 (
55548ee861, 2026-08-24,BREAKING: remove star attention) deleted star attention, and the list is now re-derived from the canonical__all__— 11 names, 9 unconditional plus 2 underIS_FLASHINFER_AVAILABLE, name-for-name and order-for-order identical to the canonical list (AST-checked). It saidgit applyof this patch againstmainfails outright: true then, and it is why the work was split into two PRs, but the patch applies cleanly now — that argument is history, not a live gate. And it claimed live in-tree evidence, which is the next section."Where is the evidence? Nothing in-tree imports these paths."
Correct, and it is the weak point, so it goes first rather than last.
At
fc8969ec59(the parent of the #17968 squash) the retired coordinates appeared 8 times across the example and the two copies of the add-a-model walkthrough: 6 import lines (examples/llm-api/out_of_tree_example/modeling_opt.py:9,15;docs/source/torch/adding_new_model.md:52,55;docs/source/models/adding-new-model.md:52,55) plus 2 prose mentions (:107in each walkthrough). #17968 repointed all of them. On today'smainthe example reads:and a grep of
mainfor either retired coordinate, outside the separate_torch/visual_gen/attention_backend/tree, returns 2 hits — both frozenblob/d6b741ddfepermalinks in an archived tech blog (blog01_…B200_GPUs.md:129,134). Zero live in-tree callers. The in-tree evidence base went from 6 lines to 0, by this Epic's own hand.What survives is out of tree and no grep here can see it. One checkable fact stands in for it: of the 93
v*tags in this repository, 61 contain that example, all 61 ship it with the old imports, and 0 carry the post-move form — fromv1.0.0rc3(2025-07-14) throughv1.3.0rc25(2026-08-27). Anyone who followed the documented recipe before 2026-09-05 holds a file whose lines 9 and 15 are exactly the two coordinates these modules restore. #17968's merged body states that between the two merges out-of-repo callers on the old paths break; that window has been open since 2026-09-05T09:48:09Z.That is the whole argument: publication history, not a measurement. I have no telemetry on out-of-tree importers and cannot put a number on them. If the team's read is that a
_torchpath carries no compatibility promise whatever the walkthrough printed, that is a coherent position — and the right outcome is to drop the two modules and land only the copyright header, which has no other home (below). I would rather have that decided than assumed."Why shim a private
_torch/path at all? These would be the first."They would be.
git grep -l "Compatibility shim for" origin/main -- tensorrt_llm/returns exactly 9 files —_common.py,serialization.py,_ray_utils.py,ray_stub.py,lora_helper.py,lora_manager.py,prompt_adapter_manager.py,executor/ray_executor.py,executor/ray_gpu_worker.py— and not one of them is under_torch/.The counter-argument is narrow, and it is not that
_torchis public. It is that this repository published these two particular_torchpaths as copy-paste code in the add-a-model walkthrough and shipped that text in 61 releases, which makes this pair de facto documented regardless of the underscore. Containment: both modules are definition-free, both warn, and removal joins the Epic's existing batch — the written policy attensorrt_llm/serialization.py:24-25is that the compatibility window "spans at least one release (Epic decision D4 (b))", tracked by T25 (TRTLLM-14855). Two of those nine —_common.pyandserialization.py— name that ticket in their docstrings; the other seven name no ticket at all. These two currently say only "will be removed in a future release" — one line each, added on request."Why didn't the MoE sibling need this?"
It didn't, and the asymmetry is conceded rather than explained away. Sibling Epic PR #17952 (
c5c985c4a1, 2026-08-30) retired_torch/modules/fused_moe/**and_torch/expert_statistic.pywith no forwarding module and no follow-up shim PR; onmainthe directory is gone andexpert_statistic.pyexists only at_torch/moe/. The criterion applied here is not "is it private" but "did a document tell users to copy it": the pre-cutfused_moereferences underdocs/were six tech-blog mentions — five source links plus one prose filename — never import lines in a template the reader is instructed to reproduce. If the team prefers one rule for the whole Epic, the consistent rule is a hard cut everywhere and this PR shrinks to the header alone."This only covers the package-root import form."
Correct, and the root form is the minority. AST census over the pre-move tree at
fc8969ec59, counting import statements, excluding everything inside the 79-file retiredattention_backend/package plusmodules/attention.py:…attention_backend.<sub>(from/import, absolute or relative)from …attention_backend import <name in __all__>from …attention_backend import <submodule object>(interface,utils,trtllm,sparse,flashinfer)from …modules.attention import Attentionfrom …modules.attention import <other>(helix helpers,extract_extra_attrs)For the package coordinate alone — the figure worth scrutinising — 83 of 414 = 20.0 %.
import tensorrt_llm._torch.attention_backend.trtllmandfrom …attention_backend.interface import AttentionMetadatastill raiseModuleNotFoundError; the retired package now holds one file.Not widened, deliberately. The shim's warrant is the walkthrough, and the walkthrough only ever taught the root form — those two published lines are exactly it. Serving the rest means re-creating a stub skeleton of the retired package's 8 submodules (79
.pyfiles pre-move) and keeping it in sync, for a surface no document ever taught.The limit points the same way and is worth stating rather than discovering: the three of #18771's fixes (
75f521ddac) that #17968 caused — the stale imports in_torch/pyexecutor/engine/lora.py,_torch/visual_gen/attention_backend/flashinfer.pyandtests/unittest/_torch/visual_gen/test_attention_flashinfer.py— were all…attention_backend.interface, submodule form. These modules would not have prevented any of them. That is the mechanism's limit, not an argument for it.CODEOWNERS: the block's rule applies, and this revision does not follow it
.github/CODEOWNERS:455-463carries a# ===== COMPATIBILITY FORWARDING MODULES =====block naming this Epic, whose stated rule is that each shim "keeps the owner its pre-move path had". The earlier body's claim that "no other shim in the tree has one" was simply wrong. Replayed with the repo's own.github/scripts/label_component.py:a56ec20396^:419,420)_torch/modules/attention.py:177)_torch/attention_backend/__init__.py:117)Straight concession: the rule is violated, not satisfied, and neither file keeps the owner its pre-move path had. The fix is two lines appended to that block, which I verified restore both paths to @NVIDIA/trt-llm-torch-attention-devs:
They are held out of this revision for one reason worth weighing rather than deciding alone:
/.github/CODEOWNERSis itself owned by @NVIDIA/trt-llm-infra-devs and @NVIDIA/trt-llm-oss-compliance (:466), so adding them pulls two more required approvals onto a 4-file PR that needs Attention-team eyes. My position is that the block wins and the two lines belong here — say the word and they go in. Meanwhile the other two changed files already route to that team by rule (:317,:321), and the team is a requested reviewer.Why the copyright header rides along
_torch/attention/backends/__init__.pycarries no NVIDIA header: 21 lines onmain, 35 with the 14-line header. Adding it during the relocation would have destroyed the rename record. Both scenarios replayed in a scratch repository:a56ec20396modules/attention.py)A+D, no rename detected at allgit blamefor the backends package root would then have pointed at the refactor instead of at the original authors. Hence: rename first, header second, here.If this PR is dropped, the header silently never arrives. The repo has no SPDX/licence pre-commit hook to notice, every hand-written sibling
__init__.pyunder_torch/attention/already carries it — 10 of the other 21, with the 11 that do not being the whole vendoredbackends/prims_ts/**subtree, which carries a FlashInfer Apache-2.0 header instead,backends/prims_ts/__init__.pyincluded — and nothing has touched this file sincea56ec20396.Residual risk
FutureWarningis unconditional at import. A downstream CI running-W error::FutureWarningturns the old path from "works with a warning" into a hard failure. Silent-by-default was rejected deliberately — a warning nobody sees migrates nobody — but the cost is real and it lands on the same out-of-tree users the shim exists for.tests/unittest/pytest.ini:14sets-W ignore::DeprecationWarningand no-W error, so the warning is visible and non-fatal here; that is our config, not theirs._torch/visual_gen/attention_backend/is a different tree and is untouched.Test Coverage
Four checks appended to
tests/unittest/_torch/attention/test_backends_importable.py(55 → 130 lines, pure insertion — no existing test edited; 8 functions total, 4 pre-existing from #18771 plus 4 new, one of them parametrized over both retired paths for 5 new cases):test_modules_attention_shim_forwards_canonical_classAttention, which silently breaksisinstanceand picklestest_attention_backend_shim_forwards_canonical_objectsis), for every name the package shim re-exportstest_attention_backend_shim_exports_match_canonical__all__and the canonical one, in either directiontest_shim_warns_on_import(×2)DeprecationWarning— which Python ignores by default outside__main__, so it would never reach the callers these modules exist forThe
__all__guard is the one that earns its keep: it is exactly the #18025 drift described above, which on today'smainsurfaces only for a caller who has FlashInfer installed — the one configuration those two names ever served. It compares sets, not ordered lists (__all__order binds nothing at import time, so freezing it would fail CI on a cosmetic reorder of the canonical list while catching no drift a caller could observe), with anassert canonical.__all__so two empty sets cannot pass vacuously. The warning check pops the module fromsys.modulesand restores both the entry and the parent-package attribute in afinally, because a module-levelwarnings.warnfires once per process and the identity checks above it have already spent it.These tests were not executed against a real
tensorrt_llm. No interpreter here can import the package — systempython3is 3.6.8 and the localpython3.11has no torch — and no usable container runtime is available. Nothing below is a CI result. What was run this round:py_compileon all four changed files: clean. Longest lines 98 / 98 / 98 / 99 against ruffline-length = 100.__all__and the canonical__all__are identical order-for-order, 11 names each.tensorrt_llm._torch→toypkgrewritten, leaf symbols stubbed andIS_FLASHINFER_AVAILABLE = Trueso the conditional branch runs. Five seeded variants, results below.ImportError: cannot import name …inside the shim's import list__all__guard fires__all__cosmetically reorderedThe warning check was confirmed order-independent three ways: inside the full run, after the identity checks have already imported both shims; alone via
-k warns; and over 5 consecutive full runs. This shows the assertions discriminate; it does not show that the real modules import.pre-commit run --filesover the four paths returned rc=0 with no hook modifying anything when the branch was built. It was not re-run for this description.No test-list change, and none is needed.
grep -rn backends_importable tests/integration/test_lists/returns 0 hits; the file is swept by the directory-levelunittest/_torch/attentionentries atl0_cpu.yml:39,l0_h100.yml:25,l0_b200.yml:108,l0_b300.yml:27,l0_dgx_b300.yml:27andl0_gb300_multi_gpus.yml:26, and adding functions to an already-swept file needs no edit. Note the file is not collected onl0_cpu:tests/unittest/conftest.py:224-240drops anytest_*.pylacking a literalpytest.mark.cpu_onlywhen a stage runs-m cpu_only, and this file carries none — nor did #18771's four checks in it. These cases therefore run on the GPU L0 attention stages. Adding the marker was left out of scope.Since no in-tree caller goes through either retired path, there is nothing else in this repository these modules could regress.
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Dev Engineer Review
Attentionimport paths.__all__, and emitFutureWarning.QA Engineer Review
Added tests:
test_modules_attention_shim_forwards_canonical_classtest_attention_backend_shim_forwards_canonical_objectstest_attention_backend_shim_exports_match_canonicaltest_shim_warns_on_importThese tests are not covered by entries in
tests/integration/test_lists/,test-db/, orqa/.Verdict: needs follow-up. Add appropriate CI or QA test-list coverage, or document why list coverage is not required.