Repository navigation
[None][refactor] define a shared multimodal model protocol - #20906
Draft
yechank-nvidia wants to merge 8 commits into
Draft
yechank-nvidia wants to merge 8 commits into
yechank-nvidia wants to merge 8 commits into
Conversation
1 task done
Signed-off-by: Yechan Kim <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: Yechan Kim <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: Yechan Kim <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: Yechan Kim <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: Yechan Kim <161688079+yechank-nvidia@users.noreply.github.com>
…cess pools Mount the existing Prometheus endpoint for encoder-only serving and publish completed request metrics through the shared extraction path. Keep process-pool media submissions picklable and test the optional encoder timing callback gate, including compiled models. Signed-off-by: Yechan Kim <161688079+yechank-nvidia@users.noreply.github.com>
Restore the CPU list entries that the original change added for the new input timing tests, so CI keeps running them after the rebase. Signed-off-by: Yechan Kim <161688079+yechank-nvidia@users.noreply.github.com>
Problem The executor decides multimodal behaviour with isinstance checks on MultimodalModelMixin, so only mixin subclasses reach the multimodal runtime paths, and the executor reaches into mixin internals (the encoder cache attribute, its private initializer and the encoder timing hook). No written contract exists that another implementation, such as a Modeling V2 target, can follow without the mixin. Solution - Add SupportsMultimodal in tensorrt_llm/_torch/models/multimodal.py: capability flags, encoder parallel modes and attention backends, language_model, embedding properties, the model-owned encoder cache, item scheduling, encoding, fusion and the encoder timing hook encoder_forward_context. The module imports no model class. - is_multimodal_model() reads the supports_multimodal class marker. get_multimodal_data_device_paths() is the one reader of the optional multimodal_data_device_paths, for any model. - Every executor gate that checked isinstance(..., MultimodalModelMixin) uses the protocol: item scheduling, the encoder cache, startup memory profiling, the breakable prefill graph check, spec-decode fast paths, the weight transfer scope and the encoder timing setup. - MultimodalModelMixin sets the marker and implements the protocol; its timing hook is public as encoder_forward_context. Cross-iteration prefetch stays a mixin feature, since it uses mixin internals. - Align the encoder entry points of six models with the mixin's Sequence input, and document the contract in adding_new_model.md and in the runtime interfaces of the Modeling V2 README. Performance No runtime change: a class-marker read replaces isinstance, and no tensor copy or host sync is added. Not measured. Validation - CPU unit tests in 21 targets (35 test files: item scheduling, encoder cache and prefetch, startup memory profiling, decoder and encoder runners, model engine and loader, LoRA, public API stability, input and encoder timing metrics), this commit vs its parent: 1871 vs 1866 cases, 1740 vs 1735 passed, 0 changed outcomes; the 5 new tests pass. The other 90 cases fail the same way on both trees: 84 need a GPU, 1 hits a CPU shared-memory error in the test container, and 5 need the pytest mocker fixture, which the harness does not load. - mypy 2.1.0: --strict passes on the protocol module, and all 21 MultimodalModelMixin subclasses conform to SupportsMultimodal. - Pre-commit hooks (check-only), py_compile and git diff --check pass. Signed-off-by: Yechan Kim <161688079+yechank-nvidia@users.noreply.github.com>
yechank-nvidia
force-pushed
the
refactor/mm-protocol-20261023
branch
from
November 12, 2026 06:31
5c0e8c7 to
253e646
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Why. The executor decides whether a model is multimodal with
isinstance(model, MultimodalModelMixin), so only subclasses of the mixin reach the multimodal runtime paths (item scheduling, the encoder cache, startup memory profiling, encoder timing). The executor also reaches into mixin internals, such as the private encoder cache initializer and the private encoder timing hook. A model that does not use the mixin, for example a Modeling V2 target, which must not import Modeling V1 model classes, has no written contract to follow.What. Add
SupportsMultimodalintensorrt_llm/_torch/models/multimodal.py: capability flags, encoder parallel modes and attention backends,language_model, embedding properties, the model-owned encoder cache, item scheduling, encoding, fusion, and the encoder timing hookencoder_forward_context. The module imports no model class.is_multimodal_model()reads thesupports_multimodalclass marker, andget_multimodal_data_device_paths()is the one reader of the optionalmultimodal_data_device_paths, for any model.Every executor gate that checked
isinstance(..., MultimodalModelMixin)now uses the protocol: item scheduling, the encoder cache, startup memory profiling, the breakable prefill graph check, spec-decode fast paths, the weight transfer scope, and the encoder timing setup.MultimodalModelMixinsets the marker and stays the shared implementation. A model that does not use the mixin opts in withsupports_multimodal = True. Cross-iteration encoder prefetch stays a mixin feature, because it uses mixin internals.Review note: until #20739 merges, this branch also carries #20739's seven commits. Please review only the last commit,
253e64679b("define a shared multimodal model protocol"). The branch will be rebased onto main once #20739 lands.Builds on #20739. #20739 (input and encoder timing metrics) adds the encoder timing hook as the private mixin attribute
_encoder_forward_contextand sets it inPyExecutor._setup_encoder_perf_metricsbehind anisinstancecheck. This PR makes the hook the public protocol memberencoder_forward_contextand gates that setup on the protocol. #20739's metrics and tests are otherwise unchanged; its two tests that set the hook use the new name.Leaves room for #21052. #21052 (encoder input batching, and copying encoder inputs to the GPU inside the encoder call) needs no protocol change. Its transfer helper
request_device_paths(model)reads the model's device paths, and it can do that throughget_multimodal_data_device_paths(model), so there is still one reader. Whichever of the two PRs lands second needs a small rebase (about ten lines: imports and the device-path readers).Also. Align the encoder entry points of six models (Gemma4, GLM-5 Next vision, Nemotron-H multimodal, Qwen2-VL, Qwen3-VL, MuseSpark) with the mixin's
Sequenceinput, so all 21 mixin subclasses conform statically. Document the contract indocs/source/torch/adding_new_model.md, and listSupportsMultimodalwithDecoderModelandDecoderModelForCausalLMamong the runtime interfaces in the Modeling V2 README.Performance. No runtime change: a class marker read replaces
isinstance, and no tensor copy or host sync is added. Not measured.Test Coverage
tests/unittest/_torch/multimodal/test_multimodal_mixin.py: detection does not evaluate encoder properties, the defaults do not enable optional features, the cache keeps the existing initialization hook, and the mixin defines every protocol member.tests/unittest/_torch/pyexecutor/engine/test_multimodal.py::test_item_scheduler_accepts_model_contract_without_mixin_inheritance: the item scheduler accepts a model that implements the protocol without the mixin.mockerfixture, which the harness does not load.--strictpasses on the protocol module, and all 21MultimodalModelMixinsubclasses conform toSupportsMultimodal.git diff --checkpass.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.