[None][feat] support mixed modality requests - #14419
Draft
venkywonka wants to merge 82 commits into
Draft
Conversation
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR implements prompt-order-aware mixed-modality support for multimodal vision-language models by introducing a universal assembly pipeline ( ChangesCore Multimodal Assembly Pipeline
Shared Multimodal Infrastructure
Nemotron Nano Model Integration
Qwen3VL Model Integration
Ordering and Embedding-Length Tests
Mixed-Modality Evaluation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tensorrt_llm/inputs/registry.py (1)
101-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow the tokenizer fallback exceptions.
In
tensorrt_llm/inputs/registry.py, the prompt/query tokenization fallback blocks catchExceptionand then assume the failure is due to the tiktoken backend not supportingadd_special_tokens. This can hide real encode bugs; restrict the fallback to the concreteTypeErrorcase (unsupportedadd_special_tokenskwarg).Suggested fix
- except Exception: + except TypeError: # Tiktoken path token_ids = self.tokenizer.encode( inputs["prompt"], allowed_special=toktoken_special_tokens) ... - except Exception: + except TypeError: # Tiktoken path query_token_ids = self.tokenizer.encode( inputs["query"], allowed_special=toktoken_special_tokens)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/inputs/registry.py` around lines 101 - 109, The current fallback around self.tokenizer.encode in tensorrt_llm/inputs/registry.py is catching all Exceptions and masking real encode errors; change the broad except Exception to catch only the concrete TypeError that indicates the tokenizer backend does not accept the add_special_tokens kwarg (i.e., use except TypeError:), and re-raise any other exceptions so real bugs aren't swallowed; keep the same fallback call using toktoken_special_tokens and sampling_params.add_special_tokens and ensure the behavior remains identical for the tiktoken path.tensorrt_llm/_torch/models/modeling_nemotron_nano.py (1)
1356-1360:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid truthiness checks on raw multimodal payloads.
mm_data["image"]can be a singletorch.Tensor, andmm_data["audio"]can be a rawnp.ndarray; both raise onbool(...). That makes the tokenized fast path fail before it even decides which expansion path to use.🛠️ Proposed fix
mm_data = mm_data or {} if mm_data: - has_image = "image" in mm_data and bool(mm_data["image"]) - has_video = "video" in mm_data and bool(mm_data["video"]) - has_audio = "audio" in mm_data and bool(mm_data["audio"]) + has_image = self._count_mm_items(mm_data, "image") > 0 + has_video = self._count_mm_items(mm_data, "video") > 0 + has_audio = self._count_mm_items(mm_data, "audio") > 0 else:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/models/modeling_nemotron_nano.py` around lines 1356 - 1360, The current truthiness checks on mm_data entries (the has_image/has_video/has_audio logic) call bool(...) on raw payloads like torch.Tensor or np.ndarray which raises; update the checks to first test presence with "in mm_data" and then check for non-None without relying on truthiness (e.g. mm_data.get("image") is not None) and, if you need to verify non-empty, use type-safe checks such as torch.is_tensor(...) or isinstance(..., np.ndarray) and explicit size/shape checks (e.g. tensor.numel() > 0 or array.size > 0) when setting has_image/has_audio; keep has_video similar but avoid bool() on raw objects so the tokenized fast path won't crash.
🧹 Nitpick comments (4)
tests/unittest/_torch/attention/test_flashinfer_attention.py (1)
67-137: QA scheduled test-list updates are unnecessary for this unittest-only addition.These changes are unit-test scope and don’t require
tests/integration/test_lists/qa/*updates.As per coding guidelines: "If the PR only touches unittest/ or narrow unit scope, say explicitly whether QA list updates are unnecessary or optional."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/attention/test_flashinfer_attention.py` around lines 67 - 137, The PR needs an explicit note that QA scheduled test-list updates are unnecessary because the change only touches unit tests (e.g., tests adding test_vswa_primary_batch_cache_indices*, FlashInferAttentionMetadata test helpers); add a one-line statement to the PR description (or commit message) saying "QA test-list updates unnecessary for unittest-only changes per guideline" and reference the guideline to make the intent explicit for reviewers.tests/integration/defs/accuracy/test_llm_api_pytorch_multimodal.py (1)
632-674: QA list follow-up for scheduled coverage.Since this change adds mixed-modality execution to
TestNanoV3Omni::test_auto_dtype, please confirm corresponding entries are added totests/integration/test_lists/qa/llm_function_core.txtso scheduled QA runs include these new functional cases. Nollm_function_multinode.txtor perf-list update is needed unless you also intend multinode/perf coverage for this path.As per coding guidelines: “all new test cases should be added [to QA functional lists]” and, for this PR’s scope,
llm_function_core.txtis the correct single-node functional list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/defs/accuracy/test_llm_api_pytorch_multimodal.py` around lines 632 - 674, Add the new mixed-modality test cases introduced in TestNanoV3Omni::test_auto_dtype (parameter ids "bf16", "fp8" and the NVFP4 variant) to the QA functional list so scheduled single-node QA picks them up; specifically append corresponding entries for these variants to tests/integration/test_lists/qa/llm_function_core.txt matching the test name TestNanoV3Omni::test_auto_dtype and the model identifiers used in the params so the scheduler will run the new mixed-modality cases.tests/unittest/inputs/test_registry_mixed_multimodal.py (1)
106-152: ⚡ Quick winAdd a repeated-modality prompt-order case.
This only exercises one item per modality, so the new flattening path never sees orders like
image[1], video[0], image[0]. A duplicate/skipped index bug in_validate_mm_item_orderor_flatten_*_by_mm_item_orderwould still pass with the current coverage.As per coding guidelines, "Assess whether new/changed tests cover happy path, important edge cases, and failure modes relevant to the feature or fix."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/inputs/test_registry_mixed_multimodal.py` around lines 106 - 152, Add a new unit test that covers a repeated-modality prompt order so the flattening/validation codepaths are exercised; specifically, in tests/unittest/inputs/test_registry_mixed_multimodal.py create a test (mirror the existing test_tokenized_wrapper_mixed_modalities_builds_metadata_for_all_items) but supply mm_data and multi_modal_uuids with multiple items for at least one modality (e.g., "image": ["img-0","img-1"], "video": ["vid-0"], "audio": ["aud-0"]) and a prompt/order that references modalities in a repeating sequence (e.g., image[1], video[0], image[0]) so the code paths in _validate_mm_item_order and the _flatten_*_by_mm_item_order helpers are executed; assert the resulting prompt_token_ids, multimodal_positions, multimodal_lengths, multimodal_uuids, and multimodal_item_run_cu_offsets match the expected flattened order to catch duplicate/skipped index bugs.tests/unittest/_torch/modeling/test_modeling_nemotron_nano_v2_vl.py (1)
909-965: No QA list update is needed for this unittest coverage.These additions stay in
tests/unittest/...and don't by themselves change scheduled QA coverage; the QA-list decision belongs to the separate integration accuracy changes elsewhere in the PR.As per coding guidelines, "If the PR only touches unittest/ or narrow unit scope, say explicitly whether QA list updates are unnecessary or optional."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/modeling/test_modeling_nemotron_nano_v2_vl.py` around lines 909 - 965, The PR adds only unit tests under tests/unittest and does not change integration or accuracy-affecting code, so explicitly state in the PR description (or changelog/PR checklist) that no QA-list update is needed; mention the specific test file tests/unittest/_torch/modeling/test_modeling_nemotron_nano_v2_vl.py and the new tests test_single_param_mixed_modalities_still_single_tensor and test_single_param_interleaved_items_follow_metadata_order to make clear these are unit-scope changes and QA-list updates are unnecessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/_torch/models/modeling_nemotron_nano.py`:
- Around line 3605-3633: The new branch that allows raw multimodal inputs on
disaggregated context workers is unreachable because __init__ still rejects
processes with TLLM_MULTIMODAL_DISAGGREGATED=1 and load_weights() skips
constructing vision_encoder/sound_encoder when _is_disagg() is true; update
__init__ to allow context-role disaggregated processes (e.g., permit
TLLM_MULTIMODAL_DISAGGREGATED when _is_disagg_context_role() is true) and change
load_weights() so that when _is_disagg_context_role() it still constructs local
encoders (vision_encoder/sound_encoder) instead of skipping them, ensuring the
checks in the raw_ctx_params branch (which reference _is_disagg(),
_is_disagg_context_role(), vision_encoder, and sound_encoder) can succeed at
runtime.
In `@tensorrt_llm/_torch/models/modeling_qwen3vl.py`:
- Around line 935-961: Validate multipart request item ordering before appending
to item_order: when reading multimodal_item_order/local_order inside the loop
that builds item_order (symbols: local_order, multimodal_item_order,
local_image_count, local_video_count, image_offset, video_offset, local_seen,
item_order), first verify that every image index in 0..local_image_count-1 and
every video index in 0..local_video_count-1 is present exactly once (no
duplicates) if the request expects full coverage, and that each index is within
bounds; if the provided local_order is partial or contains
duplicates/out-of-range indices, raise a ValueError (or fall back to
constructing the default contiguous order) instead of silently accepting it so
forward() can't be fed a corrupted item_order. Ensure checks occur before
converting entries to ints and mutating image_offset/video_offset so errors are
detected per-request.
In `@tensorrt_llm/inputs/registry.py`:
- Around line 825-841: In _validate_mm_item_order, ensure indices within each
modality are unique rather than just counting occurrences: replace or augment
the current seen counter logic with a per-modality set of seen indices (use
symbols item_order, mm_items, seen) and on each (modality, idx) check if idx is
already in seen[modality] and raise a ValueError for duplicate index (mention
the offending modality and idx); continue to validate bounds as currently done
and finally verify that the number of unique seen indices equals
len(mm_items[modality]).
In `@tests/integration/defs/accuracy/accuracy_core.py`:
- Line 326: Move parsing/validation of TRTLLM_MIXED_MOD_NUM_SAMPLES out of
module import and into the fallback path inside
MixedModality.get_hypothesis_testing_params: remove or stop using the top-level
NUM_SAMPLES = int(os.getenv(...)) and instead, when an entry lacks
"num_samples", read os.getenv("TRTLLM_MIXED_MOD_NUM_SAMPLES") then parse to int
with try/except, validate that the parsed value is > 0, and use it as the
fallback; if parsing fails or value <= 0, fall back to a safe default (e.g., 50)
and log or raise a clear error as appropriate. Ensure references to
MixedModality.NUM_SAMPLES are updated to use the new local/fallback logic inside
get_hypothesis_testing_params.
In `@tests/unittest/_torch/modeling/test_modeling_qwen3vl.py`:
- Around line 312-317: The test is assigning to the read-only property
processor.config (defined on Qwen3VLInputProcessorBase), which raises
AttributeError; instead set the underlying attribute processor._config to the
SimpleNamespace so the property returns the desired values. Update the test
test_qwen3vl_item_order_uses_vision_start_boundaries to assign the
SimpleNamespace to processor._config (not processor.config) before calling
get_mm_item_order.
---
Outside diff comments:
In `@tensorrt_llm/_torch/models/modeling_nemotron_nano.py`:
- Around line 1356-1360: The current truthiness checks on mm_data entries (the
has_image/has_video/has_audio logic) call bool(...) on raw payloads like
torch.Tensor or np.ndarray which raises; update the checks to first test
presence with "in mm_data" and then check for non-None without relying on
truthiness (e.g. mm_data.get("image") is not None) and, if you need to verify
non-empty, use type-safe checks such as torch.is_tensor(...) or isinstance(...,
np.ndarray) and explicit size/shape checks (e.g. tensor.numel() > 0 or
array.size > 0) when setting has_image/has_audio; keep has_video similar but
avoid bool() on raw objects so the tokenized fast path won't crash.
In `@tensorrt_llm/inputs/registry.py`:
- Around line 101-109: The current fallback around self.tokenizer.encode in
tensorrt_llm/inputs/registry.py is catching all Exceptions and masking real
encode errors; change the broad except Exception to catch only the concrete
TypeError that indicates the tokenizer backend does not accept the
add_special_tokens kwarg (i.e., use except TypeError:), and re-raise any other
exceptions so real bugs aren't swallowed; keep the same fallback call using
toktoken_special_tokens and sampling_params.add_special_tokens and ensure the
behavior remains identical for the tiktoken path.
---
Nitpick comments:
In `@tests/integration/defs/accuracy/test_llm_api_pytorch_multimodal.py`:
- Around line 632-674: Add the new mixed-modality test cases introduced in
TestNanoV3Omni::test_auto_dtype (parameter ids "bf16", "fp8" and the NVFP4
variant) to the QA functional list so scheduled single-node QA picks them up;
specifically append corresponding entries for these variants to
tests/integration/test_lists/qa/llm_function_core.txt matching the test name
TestNanoV3Omni::test_auto_dtype and the model identifiers used in the params so
the scheduler will run the new mixed-modality cases.
In `@tests/unittest/_torch/attention/test_flashinfer_attention.py`:
- Around line 67-137: The PR needs an explicit note that QA scheduled test-list
updates are unnecessary because the change only touches unit tests (e.g., tests
adding test_vswa_primary_batch_cache_indices*, FlashInferAttentionMetadata test
helpers); add a one-line statement to the PR description (or commit message)
saying "QA test-list updates unnecessary for unittest-only changes per
guideline" and reference the guideline to make the intent explicit for
reviewers.
In `@tests/unittest/_torch/modeling/test_modeling_nemotron_nano_v2_vl.py`:
- Around line 909-965: The PR adds only unit tests under tests/unittest and does
not change integration or accuracy-affecting code, so explicitly state in the PR
description (or changelog/PR checklist) that no QA-list update is needed;
mention the specific test file
tests/unittest/_torch/modeling/test_modeling_nemotron_nano_v2_vl.py and the new
tests test_single_param_mixed_modalities_still_single_tensor and
test_single_param_interleaved_items_follow_metadata_order to make clear these
are unit-scope changes and QA-list updates are unnecessary.
In `@tests/unittest/inputs/test_registry_mixed_multimodal.py`:
- Around line 106-152: Add a new unit test that covers a repeated-modality
prompt order so the flattening/validation codepaths are exercised; specifically,
in tests/unittest/inputs/test_registry_mixed_multimodal.py create a test (mirror
the existing
test_tokenized_wrapper_mixed_modalities_builds_metadata_for_all_items) but
supply mm_data and multi_modal_uuids with multiple items for at least one
modality (e.g., "image": ["img-0","img-1"], "video": ["vid-0"], "audio":
["aud-0"]) and a prompt/order that references modalities in a repeating sequence
(e.g., image[1], video[0], image[0]) so the code paths in
_validate_mm_item_order and the _flatten_*_by_mm_item_order helpers are
executed; assert the resulting prompt_token_ids, multimodal_positions,
multimodal_lengths, multimodal_uuids, and multimodal_item_run_cu_offsets match
the expected flattened order to catch duplicate/skipped index bugs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9921b54d-d890-4408-be76-d53b2fae7fad
📒 Files selected for processing (20)
tensorrt_llm/_torch/attention_backend/flashinfer.pytensorrt_llm/_torch/attention_backend/vanilla.pytensorrt_llm/_torch/models/modeling_nemotron_nano.pytensorrt_llm/_torch/models/modeling_qwen3vl.pytensorrt_llm/_torch/modules/fused_moe/create_moe.pytensorrt_llm/_torch/modules/fused_moe/fused_moe_cute_dsl.pytensorrt_llm/inputs/multimodal.pytensorrt_llm/inputs/registry.pytests/integration/defs/accuracy/accuracy_core.pytests/integration/defs/accuracy/mixed_modality.pytests/integration/defs/accuracy/references/mixed_modality_omni.yamltests/integration/defs/accuracy/test_llm_api_pytorch_multimodal.pytests/unittest/_torch/attention/test_flashinfer_attention.pytests/unittest/_torch/attention/test_vanilla_attention.pytests/unittest/_torch/modeling/test_modeling_nemotron_nano_v2_vl.pytests/unittest/_torch/modeling/test_modeling_qwen3vl.pytests/unittest/_torch/modeling/test_nemotron_nano_preprocessing.pytests/unittest/_torch/multimodal/test_mixed_modality_eval.pytests/unittest/inputs/test_multimodal.pytests/unittest/inputs/test_registry_mixed_multimodal.py
| ALPHA = 0.05 | ||
| BETA = 0.2 | ||
| SIGMA = 50.0 | ||
| NUM_SAMPLES = int(os.getenv("TRTLLM_MIXED_MOD_NUM_SAMPLES", "50")) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
for raw in ("50", "0", "-1", "abc"):
try:
n = int(raw)
scale = (2 * 50.0**2 / n) ** 0.5
print(raw, "=>", n, "scale_type:", type(scale).__name__)
except Exception as exc:
print(raw, "=>", type(exc).__name__, str(exc))
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/integration/defs/accuracy/accuracy_core.py"
echo "== Relevant lines around NUM_SAMPLES env parse =="
python - <<PY
import itertools
path="$FILE"
start=300
end=360
with open(path, "r", encoding="utf-8") as f:
lines=f.readlines()
for i in range(start-1, end):
if 0 <= i < len(lines):
print(f"{i+1}:{lines[i].rstrip()}")
PY
echo
echo "== Search for NUM_SAMPLES / num_samples usage in the file =="
rg -n --hidden --no-ignore -S "NUM_SAMPLES|num_samples|TRTLLM_MIXED_MOD_NUM_SAMPLES" "$FILE" || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 3813
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/integration/defs/accuracy/accuracy_core.py"
echo "== compute_theta / compute_threshold definitions =="
python - <<PY
path="$FILE"
for start,end in [(35,95),(90,140),(160,270),(780,920)]:
print("\n--- lines %d-%d ---"%(start,end))
with open(path,"r",encoding="utf-8") as f:
lines=f.readlines()
for i in range(start-1, min(end, len(lines))):
print(f"{i+1}:{lines[i].rstrip()}")
PY
echo
echo "== Find where task.NUM_SAMPLES is used for mixed_modality =="
rg -n --hidden --no-ignore "task\.NUM_SAMPLES|NUM_SAMPLES\s*=" "$FILE" | head -n 80Repository: NVIDIA/TensorRT-LLM
Length of output: 16948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/integration/defs/accuracy/accuracy_core.py"
echo "== Search for reference registrations and mixed_modality_omni =="
rg -n --hidden --no-ignore "mixed_modality_omni" "$FILE" || true
rg -n --hidden --no-ignore "self\.reference|reference\s*=" "$FILE" || true
rg -n --hidden --no-ignore "NUM_SAMPLES\s*=\s*int\(os\.getenv\(\"TRTLLM_MIXED_MOD_NUM_SAMPLES\"" "$FILE" || true
echo
echo "== Show surrounding block for 'mixed_modality_omni' matches (small window) =="
python - <<PY
import re
path="$FILE"
needle="mixed_modality_omni"
with open(path,"r",encoding="utf-8") as f:
lines=f.readlines()
for idx,line in enumerate(lines, start=1):
if needle in line:
s=max(1,idx-25); e=min(len(lines), idx+25)
print(f"\n--- {idx}:{line.rstrip()} ---")
for j in range(s,e+1):
print(f"{j}:{lines[j-1].rstrip()}")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 2142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/integration/defs/accuracy/accuracy_core.py"
python - <<'PY'
import re
path="tests/integration/defs/accuracy/accuracy_core.py"
with open(path,"r",encoding="utf-8") as f:
lines=f.readlines()
# print region around reference loading (line numbers observed: 155)
for i in range(130, 210):
if i < len(lines):
print(f"{i+1}:{lines[i].rstrip()}")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 3568
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/integration/defs/accuracy/accuracy_core.py"
rg -n --hidden --no-ignore "get_positive_int_env|positive_int|os\.getenv\(" "$FILE" || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
REF_DIR="tests/integration/defs/accuracy/references"
echo "== Files in references dir matching mixed_modality_omni =="
ls -1 "$REF_DIR" | rg -n "mixed_modality_omni" || true
YAML="$REF_DIR/mixed_modality_omni.yaml"
echo
echo "== Head of $YAML =="
python - <<PY
import itertools
path="$YAML"
try:
with open(path,"r",encoding="utf-8") as f:
for i,line in zip(range(1,251), f):
print(f"{i}:{line.rstrip()}")
except FileNotFoundError:
print("MISSING")
PY
echo
echo "== Grep for 'num_samples' within mixed_modality_omni.yaml =="
rg -n "num_samples" "$YAML" || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 1083
Avoid import-time failures for TRTLLM_MIXED_MOD_NUM_SAMPLES (parse/validate only when used)
MixedModality.NUM_SAMPLES parses TRTLLM_MIXED_MOD_NUM_SAMPLES via int(os.getenv(...)) at import time, so a non-integer env value can crash module import. Also, for existing tests/integration/defs/accuracy/references/mixed_modality_omni.yaml entries, num_samples: 50 is already set, so the env value doesn’t currently affect the hypothesis threshold math; 0/negative would only matter as a fallback if a reference entry ever omits num_samples.
Move env parsing/validation into the get_hypothesis_testing_params fallback path (when entry lacks num_samples) and enforce > 0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/defs/accuracy/accuracy_core.py` at line 326, Move
parsing/validation of TRTLLM_MIXED_MOD_NUM_SAMPLES out of module import and into
the fallback path inside MixedModality.get_hypothesis_testing_params: remove or
stop using the top-level NUM_SAMPLES = int(os.getenv(...)) and instead, when an
entry lacks "num_samples", read os.getenv("TRTLLM_MIXED_MOD_NUM_SAMPLES") then
parse to int with try/except, validate that the parsed value is > 0, and use it
as the fallback; if parsing fails or value <= 0, fall back to a safe default
(e.g., 50) and log or raise a clear error as appropriate. Ensure references to
MixedModality.NUM_SAMPLES are updated to use the new local/fallback logic inside
get_hypothesis_testing_params.
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
xinhe-nv
approved these changes
May 22, 2026
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
venkywonka
marked this pull request as draft
May 29, 2026 01:01
Captures pre-existing dirty state at the time the encode-multimodal perf redesign branch began: test additions in mixed_modality.py integration evaluator and per-model unit suites, plus a small Nano model tweak. Lands as a single baseline commit so the upcoming flat-item plan refactor stacks cleanly on top. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Adds module-level `_nano_extract_items` (with the
`_nano_item_token_count` helper) in `modeling_nemotron_nano.py`, which
walks one `MultimodalParams` and yields `MultimodalItem` instances for
the new `EncodingPlan.from_params` flat-item flow:
* Pure single-modality params yield one item at MMItemOrder rank 0.
* Mixed-modality params yield one item per modality slot, with
`item_idx_in_param` set to the item's MMItemOrder rank parsed from
`multimodal_item_order` metadata.
* Video payloads with an embedded audio track yield two items: the real
video item first (non-ghost; `token_count` is post-interleave) and a
ghost audio item second (`item_idx_in_param == -1`; `token_count` is
the raw audio rows). Non-ghost-first ordering matches the
`encode_with_plan` scatter convention.
* Unknown modalities raise `ValueError("Unknown modality: ...")`,
consistent with existing Nano encode-path error handling.
Per-item token counts are sourced from a `num_tokens` field stashed on
each modality payload, mirroring what `multimodal_embedding_lengths`
carries for mixed-modality requests.
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…ata in registry In maybe_compute_mm_embed_cumsum the local 'mm_data' held the PROCESSED multimodal_data dict — the opposite meaning of the raw 'mm_data' (= multi_modal_data) used at ~100 other sites in this module. Rename the local and its four uses to 'multimodal_data' and add a one-line note, removing the same-name-opposite-meaning trap. No behavior change; scoped to one function. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…egate to it Nano and Qwen3VL _get_mm_item_order_from_text were two copies of the same left-to-right placeholder walk over a decoded prompt, differing only in placeholder strings. Extract the walk into scan_prompt_order(text, placeholder_by_modality, expected_counts) in inputs/multimodal.py and have both models delegate. The shared scanner accepts a single placeholder string or an iterable per modality, skips falsy placeholders, picks the nearest match from a moving cursor (longer placeholder wins a position tie), and raises ValueError if any modality's observed count differs from expected. This is the behavioral union of both prior copies, so each caller is preserved; the per-model ValueError message is unified to one shape. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…metadata The tokenized fast path (call_with_token_ids) ran the text path on synthetic modality-major placeholder text, which for a mixed request baked a modality-major multimodal_item_order into the processed metadata. resolve_order then short-circuited on that stale metadata and never scanned the real prompt token IDs, yielding a wrong order for interleaved prompts (e.g. image,video,image resolved as image,image,video). Drop the stale dummy multimodal_item_order before resolving, then bake the order resolved from the real prompt token IDs back into extra_processed_inputs[multimodal_data] using the same wire format the text path emits. The fix is in the shared registry seam (a top-level order key cannot ride the nested mm_data_updates merge, and the stale-order shadow is model-agnostic). Tests: extend the fake-processor token-id test and add an interleaved real-Nano image,video,image case asserting the baked order matches the real prompt order. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…ed get_mm_item_order hook Every mixed preprocess path now bakes multimodal_item_order into the processed metadata: the text paths always did, and the token-id fast path does after expanding from the real prompt token IDs. So MixedModalEncodeContext.resolve_order no longer needs to parse prompts via a duck-typed get_mm_item_order hook on the input processor: it is purely metadata | default_order. Delete the hook branch and the now-unused prompt_token_ids and input_processor parameters. The hashing wrapper (create_input_processor_with_hash) drops the stale args and resolves from metadata. The tokenized fast path (call_with_token_ids) still needs the real interleaved order to project per-item token lengths before expansion, so it now derives that order from the model's own get_mm_item_order hook directly (single-modality keeps the modality-major default), then validates and re-bakes it. get_mm_item_order remains a model-internal method (still called by Nano _expand_mixed_prompt_token_ids_for_mm), no longer a framework hook. Tests: retarget the resolve_order hook-present/absent tests to metadata-present/absent; update the registry fake processor and the Nano token-path test to capture order via the model hook instead of the (now metadata-only) resolve_order. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…ncoder adapters -> _<modality>_encoder_adapter Behavior-preserving naming cleanup. In mixed_modal_encode.py, rename the per-modality output dict param/local bucket_outputs to outputs_by_modality in scatter_to_prompt_order and encode_by_modality_and_scatter. In modeling_nemotron_nano.py, rename _adapter_vision_bucket -> _vision_encoder_adapter and _adapter_audio_bucket -> _audio_encoder_adapter (def sites, encoders dict, docstring). Update the Nano test references to match. Removes the two inline TODO comments. No behavior change. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…er_adapter (convention parity with Nano) Behavior-preserving naming cleanup for convention parity with the Nano model. In modeling_qwen3vl.py, rename _adapter_image_bucket -> _image_encoder_adapter and _adapter_video_bucket -> _video_encoder_adapter (def sites and the forward() encoders dict). Update the Qwen3VL extractor test references to match. No behavior change. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…alidate_coverage/flatten/flatten_uuids) validate_coverage had zero callers; the flatten/flatten_uuids instance methods were test-only thin wrappers over the static project_by_order/ project_uuids_by_order (production already calls the static methods in registry.py, modeling_nemotron_nano.py, and modeling_qwen3vl.py). Drop all three plus the two tests that existed only to cover the instance wrappers, and the stale class docstring line referencing them. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
… Nano consumer find_multimodal_embedding_lengths was made public in inputs/multimodal.py for callers outside the module, but had exactly one caller: the Nano NanoV2VLInputProcessor._process_mixed_modalities path. Move it onto that processor as a private method _find_multimodal_embedding_lengths (dropping the redundant input_processor param, which was always self), reducing the public surface. Its private mask deps (_as_cpu_tensor, _compute_mm_masks, _find_mm_embedding_lengths_from_masks) stay in multimodal.py since registry.py also uses them; the Nano module now imports them directly. Behavior-identical. Updates the nano preprocessing test to patch the new method via mock.patch.object and refreshes two docstring/comment references. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…context __post_init__ owns the invariant The PER-ITEM branch of _nano_extract_items pre-checked len(item_order) != len(embedding_lengths) right before building a MixedModalEncodeContext, duplicating the invariant that MixedModalEncodeContext.__post_init__ already enforces. Delete the redundant len-mismatch pre-guard (and the now-unused item_order local) and let the constructor own the invariant. The None guard for a present multimodal_item_order key with absent multimodal_embedding_lengths is intentionally kept: dropping it would let from_metadata reach tuple(int(x) for x in None) and raise an opaque TypeError instead of a clear KeyError. Update test_multi_modality_embedding_lengths_mismatch_raises to match the constructor message (embedding_lengths length) instead of the deleted pre-guard message (multimodal_embedding_lengths). Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…_payload _slice_payload was a pure passthrough with an identical signature whose body just forwarded to _nano_slice_payload. Delete it and have the sole user (_nano_extract_items wiring) pass self._nano_slice_payload directly. Update test helpers and docstrings to bind/reference _nano_slice_payload. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Make video-embedded-audio promotion elegant without changing behavior. registry.py: add a single module-level _promote_nested_mm_data(processor, mm_data) that owns the optional-hook getattr, and route the hashing-path site (create_input_processor_with_hash) through it. call_with_token_ids keeps calling self.promote_nested_mm_data directly (self is always a BaseMultimodalInputProcessor, so the hook is guaranteed) and documents why. The base hook docstring now states the idempotency and non-mutating per-boundary contract. modeling_nemotron_nano.py: add a plain _video_embedded_audios(mm_data) helper returning the per-video embedded-audio list (or [] when no audio extractor), and use it for the early-out scan in both promote_nested_mm_data and _hoist_video_embedded_audio, removing the duplicated .audio scan and the scattered audio-extractor guard. The encode path is untouched. Adds focused unit tests for the new helper and promote idempotency. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…elper Add public compute_mm_embedding_lengths() in tensorrt_llm/inputs/multimodal.py as the single source of truth for the mask -> per-item embedding-length derivation. It wraps the existing _compute_mm_masks -> _find_mm_embedding_lengths_from_masks pair behind one entry point. Nano: delete the bespoke NanoV2VLInputProcessor._find_multimodal_embedding_lengths method and call the shared helper at its sole call site; drop the now-unused private imports (_as_cpu_tensor, _compute_mm_masks, _find_mm_embedding_lengths_from_masks) and import compute_mm_embedding_lengths instead. Registry: guard the lengths recompute behind a missing-key check so a processor that pre-bakes multimodal_embedding_lengths (e.g. Nano) is not double-computed; the masks are still derived (and reused) for start positions, runs, and the embed cumsum. setdefault already would not overwrite, so this is a no-double-compute clarity/perf change and is behavior-preserving. Tests: extend test_multimodal.py with public-helper coverage (mask-equivalence, the vocab-size route, and the empty-input case) and add a registry guard-preservation test asserting a processor-baked lengths value survives the hashing path unchanged. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
The Qwen3-VL mixed-modality branch (len(modalities) > 1) baked
multimodal_item_order but never multimodal_embedding_lengths. On the non-hashing
(direct preprocess) path nobody filled the lengths key, so the per-item extractor
built a MixedModalEncodeContext via
from_metadata(..., multimodal_data.get("multimodal_embedding_lengths")) and hit
tuple(int(x) for x in None) -> TypeError, crashing mixed image+video requests.
Hoist the scanned item order to a local in the mixed branch, then after
fused_input_ids is finalized (post-_postprocess) and the image/video keys are
populated, derive per-item embedding lengths with the existing generic helpers:
find_mm_token_lengths -> project_by_order -> compute_mm_embedding_lengths. This
mirrors exactly what the registry hashing path already does for Qwen, so the
non-hashing path now emits identical metadata.
Add a CPU-only preprocess test (hashing disabled) asserting the mixed branch
bakes multimodal_embedding_lengths in prompt order and that the resulting
metadata constructs a MixedModalEncodeContext via from_metadata without error.
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
… (l0_l40s collection failure) Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
… assert->ValueError On the Nano dynamic-tiler mixed path, image rows were planned at the full _max_num_patches cap (get_num_tokens_per_image) while the encoder tiled at the reservation-reduced budget (_process_images_dynamic). When the budget squeezed a large image below the cap (large image co-located with video/audio, or a tight max_model_len), the planned count exceeded the actual encoder rows, so the encoder emitted fewer rows than sum(item.rows) and encoder_dispatch raised (or, under python -O with the assert compiled out, the mismatch silently corrupted the index_copy_ scatter). Defer image entries as a None sentinel in _get_num_tokens_for_item_order when the dynamic tiler is active, then splice the actual post-tile num_tokens_per_image (plus the img start/end specials, matching get_num_tokens_per_image semantics) into those slots in _process_mixed_modalities before building input_ids and multimodal_embedding_lengths. A loud-fail ValueError guards any unreconciled slot. The fixed-tile path keeps its direct get_num_tokens_per_image call and is unchanged. Also convert the encoder_dispatch row-count assert to a ValueError so the mismatch cannot be compiled out under python -O and silently corrupt the scatter. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
… the Nano token-id path The tokenized fast path (call_with_token_ids) runs the text path on synthetic modality-major placeholder text, so any multimodal_embedding_lengths the processor pre-bakes (Nano, via compute_mm_embedding_lengths) lands in stale modality-major order. The path drops and re-bakes the stale multimodal_item_order but never touched multimodal_embedding_lengths -- its lengths-shaped twin. The hashing-path guard only recomputes lengths when absent, so a surviving stale list is kept; and consumers index lengths by the interleaved item order, reading the wrong item row count. Re-bake multimodal_embedding_lengths in the real interleaved prompt order right where the item order is re-baked, recomputing from the real expanded prompt IDs and real-order num_mm_tokens via compute_mm_embedding_lengths (mirroring the hashing-path mask derivation). Re-bake rather than pop-and-defer to the hashing recompute because a non-hashing path reaches the consumer (multimodal_hashing_supported False, or a first hashing attempt raising, makes process_prompt_maybe_hash call the processor directly), where that recompute never runs -- popping would leave lengths absent, re-introducing the None-lengths class. Guard on vocab_size/mm_token_ids availability (skip when neither is exposed). Test: interleaved Nano-style token-id request whose two items have unequal embed lengths, asserting the baked lengths track the real interleaved order, not the stale modality-major dummy order. Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
…mpt_text_for_mm to avoid a duplicate order scan Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Collaborator
Author
|
/bot run |
Resolve 3 additive-only test-file conflicts: - tests/integration/test_lists/test-db/l0_l40s.yml: keep both our MM encode test entries and upstream's qwen3vl quant-config + step3p7vl entries - tests/unittest/_torch/modeling/test_modeling_qwen3vl.py: keep both MultimodalParams import (ours) and QuantConfig/QuantAlgo imports (upstream) - tests/integration/defs/accuracy/test_llm_api_pytorch_multimodal.py: keep TestNanoV3Omni::test_mixed_modality (ours) and new TestStep3_7 class (upstream) Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
Collaborator
Author
|
/bot kill |
Collaborator
Author
|
/bot run |
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Collaborator
|
PR_Github #52112 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #52113 [ kill ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #52112 [ run ] completed with state |
Collaborator
|
PR_Github #52113 [ kill ] completed with state |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
tests/integration/defs/accuracy/references/mixed_modality_omni.yaml (1)
12-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign FP8/NVFP4 reference accuracy with the reported mixed-run values.
The PR objective reports mixed target accuracy of 70.0% for FP8 and NVFP4, but this file records 72.00 for both. Please either align these values or document why a stricter floor is intentional.
Suggested fix
nvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-FP8: - quant_algo: FP8 kv_cache_quant_algo: FP8 - accuracy: 72.00 + accuracy: 70.00 num_samples: 50 nvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-NVFP4: - quant_algo: MIXED_PRECISION kv_cache_quant_algo: FP8 - accuracy: 72.00 + accuracy: 70.00 num_samples: 50🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/defs/accuracy/references/mixed_modality_omni.yaml` around lines 12 - 21, Update the FP8/NVFP4 reference accuracies in the YAML entries for nvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-FP8 and nvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-NVFP4 by changing the accuracy field from 72.00 to the reported mixed-run value 70.00 (or alternatively add a brief comment explaining why a stricter 72.00 floor is intentional), ensuring the accuracy fields for those two model keys reflect the PR's 70.0% target.tests/integration/defs/accuracy/accuracy_core.py (1)
448-452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the defensive env parser here to avoid import-time crashes.
Line 448 still uses raw
int(os.getenv(...)); a non-integer env value can raise during module import. Reuse_env_num_samples(...)like the other mixed-modality tasks.Suggested fix
- NUM_SAMPLES = int( - os.getenv( - "TRTLLM_QWEN_VL_MIXED_MOD_NUM_SAMPLES", - os.getenv("TRTLLM_MIXED_MOD_NUM_SAMPLES", "50"), - )) + NUM_SAMPLES = _env_num_samples( + "TRTLLM_QWEN_VL_MIXED_MOD_NUM_SAMPLES", + "TRTLLM_MIXED_MOD_NUM_SAMPLES", + )#!/bin/bash set -euo pipefail FILE="tests/integration/defs/accuracy/accuracy_core.py" echo "== Verify mixed-modality NUM_SAMPLES declarations ==" rg -n "class QwenVLMixedModality|NUM_SAMPLES = " "$FILE" echo echo "== Show exact Qwen NUM_SAMPLES block ==" sed -n '440,455p' "$FILE" echo echo "== Show helper usage in other mixed-modality classes ==" sed -n '352,410p' "$FILE"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/defs/accuracy/accuracy_core.py` around lines 448 - 452, Replace the raw int(os.getenv(...)) used to set NUM_SAMPLES in the QwenVLMixedModality block with the defensive helper _env_num_samples so non-integer env values won't crash at import; call _env_num_samples with the same primary and fallback env keys ("TRTLLM_QWEN_VL_MIXED_MOD_NUM_SAMPLES" and "TRTLLM_MIXED_MOD_NUM_SAMPLES") and a default of 50, assigning its return to NUM_SAMPLES.
🧹 Nitpick comments (5)
tensorrt_llm/_torch/models/modeling_qwen3vl.py (1)
224-229: 💤 Low valueRemove draft implementation notes from docstring.
The "Option Y:" paragraph appears to be planning text rather than user-facing documentation. Consider removing or moving to a comment if this context is still relevant for maintainers.
📝 Suggested cleanup
- Option Y: no `MultimodalParams` field is added and the dict keys stay the wire - format; the context is built transiently here and lives only inside the - extractor. `from_metadata` returns None for pure single-modality requests (no - `multimodal_item_order` key); the modality-major default order is synthesized - in that case over the real per-modality sub-item counts, so a multi-item - single-modality request (e.g. 2 images) enumerates every sub-item.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/models/modeling_qwen3vl.py` around lines 224 - 229, Remove the draft "Option Y:" paragraph from the user-facing docstring in tensorrt_llm._torch.models.modeling_qwen3vl (the docstring that mentions MultimodalParams, from_metadata, multimodal_item_order, and extractor); either delete that planning text entirely or convert it into an inline code comment adjacent to the implementation that uses from_metadata/extractor so maintainers keep the note but it is not exposed to users, and ensure the remaining docstring retains only concise, user-facing documentation.tests/integration/test_lists/test-db/l0_h100.yml (1)
357-362: Coverage status: sufficient for this PR’s post-merge mixed-modality gates.
tests/integration/test_lists/test-db/l0_h100.ymlnow includes both Qwen mixed-modality budgets and Nano mixed-modality EVS on/off × budget permutations, which is a strong post-merge safety net for the new path.As per coding guidelines:
tests/**: “Keep feedback actionable ... whether coverage is sufficient, insufficient, or needs follow-up outside the PR.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_lists/test-db/l0_h100.yml` around lines 357 - 362, Update tests/integration/test_lists/test-db/l0_h100.yml to explicitly include the listed mixed-modality permutations: the Qwen entries (accuracy/test_llm_api_pytorch_multimodal.py::TestQwen3VL::test_mixed_modality with budgets full_budget and forced_chunked_prefill) and the Nano entries (accuracy/test_llm_api_pytorch_multimodal.py::TestNanoV3Omni::test_mixed_modality with budgets full_budget and forced_chunked_prefill combined with evs_off and evs_on), and keep a short comment noting coverage is sufficient for post-merge mixed-modality gates per tests/** guidelines; ensure the YAML syntax and job identifiers exactly match the six lines shown so CI will run those permutations.tests/integration/test_lists/test-db/l0_l40s.yml (1)
26-28: Coverage status: sufficient for pre-merge unit-level mixed-modality validation.Adding
test_mixed_modal_encode.py,test_nano_encode_extractor.py, andtest_qwen3vl_encode_extractor.pyintests/integration/test_lists/test-db/l0_l40s.ymlgives good early regression coverage for the new encode/extractor surfaces.As per coding guidelines:
tests/**: “Keep feedback actionable ... whether coverage is sufficient, insufficient, or needs follow-up outside the PR.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_lists/test-db/l0_l40s.yml` around lines 26 - 28, Annotate the YAML entry that adds unittest/_torch/modeling/test_mixed_modal_encode.py, test_nano_encode_extractor.py, and test_qwen3vl_encode_extractor.py with an explicit coverage status so reviewers can see the actionable verdict; update tests/integration/test_lists/test-db/l0_l40s.yml by adding a short metadata field or inline comment next to those three filenames indicating "coverage: sufficient for pre-merge unit-level mixed-modality validation" (or similar), so the list clearly states whether coverage is sufficient/insufficient/needs follow-up for these new encode/extractor tests.tests/unittest/_torch/multimodal/test_mixed_modality_eval.py (1)
105-402: ⚡ Quick winCoverage is still insufficient for MMMU multi-image rows.
tests/unittest/_torch/multimodal/test_mixed_modality_eval.pycovers prompt/scoring/selection well, but it never builds an MMMU row with multiple populatedimage*fields. Please add a regression here (or a small loader-focused companion test) that provestests/integration/defs/accuracy/mixed_modality.pyeither rejects those rows or preserves all images intentionally.As per coding guidelines,
tests/**:Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM. Keep feedback actionable: suggest concrete list file names and whether coverage is sufficient, insufficient, or needs follow-up outside the PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/multimodal/test_mixed_modality_eval.py` around lines 105 - 402, Add a regression test that constructs an MMMU-style sample with multiple populated image* fields (using helpers like _input_samples_by_modality or _sample to build the row) and assert the evaluator/loader behavior: call MixedModalityEvaluator or select_mixed_modality_samples and verify either the row is rejected (raises ValueError) or all image fields are preserved and appear in the resulting sample/prompt (e.g., _format_mixed_modality_prompt contains each image question or the sampler returns items containing all image modalities); place this as a new test function (e.g., test_mmmu_multi_image_rows_rejected_or_preserved) alongside existing tests so CI will cover the regression.tests/unittest/inputs/test_mixed_modal_encode_context.py (1)
105-118: ⚡ Quick winCoverage is insufficient for raw single-video payloads.
This only exercises
videoas an already-itemized list. Please add a regression here for the raw shape{"video": [frame0, frame1]}and a matching end-to-end case intests/unittest/inputs/test_registry_mixed_multimodal.py, so normalization/order resolution and the hashing path both cover the single-video frame-list form this PR now relies on.As per coding guidelines,
tests/**: Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM. Keep feedback actionable: suggest concrete list file names and whether coverage is sufficient, insufficient, or needs follow-up outside the PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/inputs/test_mixed_modal_encode_context.py` around lines 105 - 118, Add a regression to TestNormalize.test_normalize_wraps_scalars_and_drops_non_modalities to cover the raw single-video-frame-list shape: call MixedModalItemOrder._normalize with a payload where "video" is a raw list of frames (e.g., [frame0, frame1]) and assert the normalized result wraps that frame-list into the expected per-item list form (use the same assertion style as the existing test); then add an end-to-end case in tests/unittest/inputs/test_registry_mixed_multimodal.py that submits the same raw-video-frame-list payload through the registry/encoding and hashing path to assert consistent ordering/hash behavior. This ensures MixedModalItemOrder._normalize and the hashing flow both cover the single-video frame-list form; tests currently insufficiently cover that shape and should be added with these two concrete test additions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/_torch/models/modeling_nemotron_nano.py`:
- Around line 1576-1617: In _get_mm_item_order_from_token_ids detect and reject
any MM placeholder that appears after its expected count is exhausted: when
matching the multi-token video pattern
(prompt_token_ids[idx:idx+video_pattern_len] == video_pattern) return None if
actual_counts["video"] >= expected_counts["video"]; similarly for single-token
modalities (image using self.img_context_token_id and audio using
self._sound_context_token_id) if token_id equals the placeholder but
actual_counts[modality] >= expected_counts[modality] return None immediately;
this ensures extra placeholders cause a fast fallback instead of being treated
as plain text.
In `@tensorrt_llm/inputs/multimodal.py`:
- Around line 1402-1424: The function _find_mm_embedding_lengths_from_masks
should raise an explicit exception when the multimodal mask positions count
doesn't match the sum of num_mm_tokens instead of using an assert; replace the
assert that checks mm_positions.numel() == lengths_t.sum().item() with an if
that raises a ValueError including mm_positions.numel(), lengths_t.sum().item(),
and the offending num_mm_tokens list (use the same message text), leaving the
early-return for empty mm_mask and the subsequent computation using
mm_positions, offset, and embed_mask unchanged.
In `@tests/integration/defs/accuracy/mixed_modality.py`:
- Around line 587-600: The current loop uses _get_mmmu_image() which only
returns the first populated image field, causing multi-image comparison rows to
be treated as single-image samples; change the logic to detect rows with more
than one populated image field and skip them (safe minimal fix) or alternatively
update _get_mmmu_image to return all images and pass a list into
MixedModalityInputSample. Concretely, in the loop that calls
_iter_mmmu_rows(dataset_path) and uses _get_mmmu_image(row), count populated
image* fields on the row (or modify _get_mmmu_image to return a list and check
its length); if more than one image is present, continue (skip the row);
otherwise proceed as before using the single image, leaving
_format_mmmu_question, MixedModalityInputSample, and _get_media_source_paths
unchanged.
In `@tests/unittest/_torch/modeling/test_nano_encode_extractor.py`:
- Line 308: The pytest.raises call using
match="MixedModalItemOrder.*multimodal_item_order" should use a raw regex string
to avoid RUF043; update the test assertion (the pytest.raises(...) call in
test_nano_encode_extractor.py that checks for KeyError) to use a raw string
literal (r"MixedModalItemOrder.*multimodal_item_order") so the
backslashes/metacharacters are interpreted as regex correctly.
In `@tests/unittest/_torch/modeling/test_nemotron_nano_preprocessing.py`:
- Around line 2671-2826: Add a test to exercise the "standalone audio +
video-embedded audio" rejection on the text path: create a proc via
TestVideoWithEmbeddedAudioHoist._make_proc(), build mm_data containing a
top-level "audio" list (e.g., one AudioData) and a "video" list with a video
that has .audio set (use _video_with_audio()), then call
proc._hoist_video_embedded_audio("<video>", mm_data) and assert that it raises
(i.e., the hoist should reject the ambiguous combination). Reference the
existing test class and the methods _hoist_video_embedded_audio, _make_proc, and
_video_with_audio when adding this new test case.
- Around line 2244-2248: The test currently only asserts the length of
multimodal_data["multimodal_embedding_lengths"]; update it to assert the exact
per-slot split to prevent regressions (e.g., verify the list equals the expected
sequence described in the comment: video-context and audio-context budgets and
any slot-specific lengths). Locate the assertion around variable lengths
(multimodal_data["multimodal_embedding_lengths"]) in
test_nemotron_nano_preprocessing.py and replace or augment the len(...) check
with an assertion that the list's contents match the expected list of integers
(the explicit per-slot embedding lengths), so changes that reorder or merge
video/audio budgets will fail the test.
In `@tests/unittest/inputs/test_registry_mixed_multimodal.py`:
- Around line 496-497: The zip in the loop that builds expanded tokens uses
zip(order, num_mm_tokens_per_placeholder) which can silently truncate mismatched
lengths; update the loop in
tests/unittest/inputs/test_registry_mixed_multimodal.py where the
comprehension/loop "for (modality, _), count in zip(order,
num_mm_tokens_per_placeholder):" appears to call zip(..., strict=True) so the
test will raise if the two iterables differ in length, making mismatches fail
loudly.
---
Duplicate comments:
In `@tests/integration/defs/accuracy/accuracy_core.py`:
- Around line 448-452: Replace the raw int(os.getenv(...)) used to set
NUM_SAMPLES in the QwenVLMixedModality block with the defensive helper
_env_num_samples so non-integer env values won't crash at import; call
_env_num_samples with the same primary and fallback env keys
("TRTLLM_QWEN_VL_MIXED_MOD_NUM_SAMPLES" and "TRTLLM_MIXED_MOD_NUM_SAMPLES") and
a default of 50, assigning its return to NUM_SAMPLES.
In `@tests/integration/defs/accuracy/references/mixed_modality_omni.yaml`:
- Around line 12-21: Update the FP8/NVFP4 reference accuracies in the YAML
entries for nvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-FP8 and
nvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-NVFP4 by changing the accuracy
field from 72.00 to the reported mixed-run value 70.00 (or alternatively add a
brief comment explaining why a stricter 72.00 floor is intentional), ensuring
the accuracy fields for those two model keys reflect the PR's 70.0% target.
---
Nitpick comments:
In `@tensorrt_llm/_torch/models/modeling_qwen3vl.py`:
- Around line 224-229: Remove the draft "Option Y:" paragraph from the
user-facing docstring in tensorrt_llm._torch.models.modeling_qwen3vl (the
docstring that mentions MultimodalParams, from_metadata, multimodal_item_order,
and extractor); either delete that planning text entirely or convert it into an
inline code comment adjacent to the implementation that uses
from_metadata/extractor so maintainers keep the note but it is not exposed to
users, and ensure the remaining docstring retains only concise, user-facing
documentation.
In `@tests/integration/test_lists/test-db/l0_h100.yml`:
- Around line 357-362: Update tests/integration/test_lists/test-db/l0_h100.yml
to explicitly include the listed mixed-modality permutations: the Qwen entries
(accuracy/test_llm_api_pytorch_multimodal.py::TestQwen3VL::test_mixed_modality
with budgets full_budget and forced_chunked_prefill) and the Nano entries
(accuracy/test_llm_api_pytorch_multimodal.py::TestNanoV3Omni::test_mixed_modality
with budgets full_budget and forced_chunked_prefill combined with evs_off and
evs_on), and keep a short comment noting coverage is sufficient for post-merge
mixed-modality gates per tests/** guidelines; ensure the YAML syntax and job
identifiers exactly match the six lines shown so CI will run those permutations.
In `@tests/integration/test_lists/test-db/l0_l40s.yml`:
- Around line 26-28: Annotate the YAML entry that adds
unittest/_torch/modeling/test_mixed_modal_encode.py,
test_nano_encode_extractor.py, and test_qwen3vl_encode_extractor.py with an
explicit coverage status so reviewers can see the actionable verdict; update
tests/integration/test_lists/test-db/l0_l40s.yml by adding a short metadata
field or inline comment next to those three filenames indicating "coverage:
sufficient for pre-merge unit-level mixed-modality validation" (or similar), so
the list clearly states whether coverage is sufficient/insufficient/needs
follow-up for these new encode/extractor tests.
In `@tests/unittest/_torch/multimodal/test_mixed_modality_eval.py`:
- Around line 105-402: Add a regression test that constructs an MMMU-style
sample with multiple populated image* fields (using helpers like
_input_samples_by_modality or _sample to build the row) and assert the
evaluator/loader behavior: call MixedModalityEvaluator or
select_mixed_modality_samples and verify either the row is rejected (raises
ValueError) or all image fields are preserved and appear in the resulting
sample/prompt (e.g., _format_mixed_modality_prompt contains each image question
or the sampler returns items containing all image modalities); place this as a
new test function (e.g., test_mmmu_multi_image_rows_rejected_or_preserved)
alongside existing tests so CI will cover the regression.
In `@tests/unittest/inputs/test_mixed_modal_encode_context.py`:
- Around line 105-118: Add a regression to
TestNormalize.test_normalize_wraps_scalars_and_drops_non_modalities to cover the
raw single-video-frame-list shape: call MixedModalItemOrder._normalize with a
payload where "video" is a raw list of frames (e.g., [frame0, frame1]) and
assert the normalized result wraps that frame-list into the expected per-item
list form (use the same assertion style as the existing test); then add an
end-to-end case in tests/unittest/inputs/test_registry_mixed_multimodal.py that
submits the same raw-video-frame-list payload through the registry/encoding and
hashing path to assert consistent ordering/hash behavior. This ensures
MixedModalItemOrder._normalize and the hashing flow both cover the single-video
frame-list form; tests currently insufficiently cover that shape and should be
added with these two concrete test additions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0084f1cf-0c39-428d-b952-8b95174dca4e
📒 Files selected for processing (26)
tensorrt_llm/_torch/models/mixed_modal_encode.pytensorrt_llm/_torch/models/modeling_nemotron_nano.pytensorrt_llm/_torch/models/modeling_qwen3vl.pytensorrt_llm/inputs/multimodal.pytensorrt_llm/inputs/registry.pytests/integration/defs/accuracy/accuracy_core.pytests/integration/defs/accuracy/mixed_modality.pytests/integration/defs/accuracy/references/mixed_modality_omni.yamltests/integration/defs/accuracy/references/mixed_modality_qwen_vl.yamltests/integration/defs/accuracy/references/mixed_modality_video_audio_omni.yamltests/integration/defs/accuracy/test_llm_api_pytorch_multimodal.pytests/integration/test_lists/test-db/l0_h100.ymltests/integration/test_lists/test-db/l0_l40s.ymltests/unittest/_torch/modeling/_mm_encode_helpers.pytests/unittest/_torch/modeling/test_mixed_modal_encode.pytests/unittest/_torch/modeling/test_modeling_nemotron_nano_v2_vl.pytests/unittest/_torch/modeling/test_modeling_qwen3vl.pytests/unittest/_torch/modeling/test_nano_encode_extractor.pytests/unittest/_torch/modeling/test_nemotron_nano_preprocessing.pytests/unittest/_torch/modeling/test_qwen3vl_encode_extractor.pytests/unittest/_torch/multimodal/test_mixed_modality_eval.pytests/unittest/inputs/test_mixed_modal_encode_context.pytests/unittest/inputs/test_multimodal.pytests/unittest/inputs/test_registry_mixed_multimodal.pytests/unittest/inputs/test_scan_prompt_order.pytests/unittest/llmapi/test_llm_kv_cache_events.py
💤 Files with no reviewable changes (1)
- tests/unittest/llmapi/test_llm_kv_cache_events.py
Comment on lines
+1576
to
+1617
| def _get_mm_item_order_from_token_ids( | ||
| self, prompt_token_ids: List[int], mm_data: Dict[str, Any] | ||
| ) -> Optional[List[Tuple[str, int]]]: | ||
| """Infer Nano item order directly from placeholder token IDs when possible.""" | ||
| expected_counts = { | ||
| modality: self._count_mm_items(mm_data, modality) for modality in _NANO_MODALITIES | ||
| } | ||
| actual_counts = {modality: 0 for modality in _NANO_MODALITIES} | ||
| item_order: List[Tuple[str, int]] = [] | ||
| video_pattern = self._video_placeholder_token_ids | ||
| video_pattern_len = len(video_pattern) | ||
|
|
||
| idx = 0 | ||
| while idx < len(prompt_token_ids): | ||
| if ( | ||
| expected_counts["video"] > actual_counts["video"] | ||
| and video_pattern | ||
| and prompt_token_ids[idx : idx + video_pattern_len] == video_pattern | ||
| ): | ||
| item_order.append(("video", actual_counts["video"])) | ||
| actual_counts["video"] += 1 | ||
| idx += video_pattern_len | ||
| continue | ||
| token_id = prompt_token_ids[idx] | ||
| if ( | ||
| expected_counts["image"] > actual_counts["image"] | ||
| and token_id == self.img_context_token_id | ||
| ): | ||
| item_order.append(("image", actual_counts["image"])) | ||
| actual_counts["image"] += 1 | ||
| elif ( | ||
| expected_counts["audio"] > actual_counts["audio"] | ||
| and self._sound_context_token_id is not None | ||
| and token_id == self._sound_context_token_id | ||
| ): | ||
| item_order.append(("audio", actual_counts["audio"])) | ||
| actual_counts["audio"] += 1 | ||
| idx += 1 | ||
|
|
||
| if actual_counts != expected_counts: | ||
| return None | ||
| return item_order |
Contributor
There was a problem hiding this comment.
Reject extra MM placeholders during token-order scanning.
Once actual_counts[modality] reaches expected_counts[modality], another <image>, <video>, or audio placeholder is treated as plain text here. In a tokenized mixed request with more placeholders than mm_data items, get_mm_item_order() can still return a “valid” order, and the later mixed expansion path will copy the trailing placeholder token(s) into expanded instead of failing fast. Please return None (to force the text fallback) or raise as soon as you detect an extra recognized placeholder after the expected count is exhausted.
Suggested fix
idx = 0
while idx < len(prompt_token_ids):
if (
- expected_counts["video"] > actual_counts["video"]
- and video_pattern
- and prompt_token_ids[idx : idx + video_pattern_len] == video_pattern
+ video_pattern
+ and prompt_token_ids[idx : idx + video_pattern_len] == video_pattern
):
+ if actual_counts["video"] >= expected_counts["video"]:
+ return None
item_order.append(("video", actual_counts["video"]))
actual_counts["video"] += 1
idx += video_pattern_len
continue
token_id = prompt_token_ids[idx]
- if (
- expected_counts["image"] > actual_counts["image"]
- and token_id == self.img_context_token_id
- ):
+ if token_id == self.img_context_token_id:
+ if actual_counts["image"] >= expected_counts["image"]:
+ return None
item_order.append(("image", actual_counts["image"]))
actual_counts["image"] += 1
elif (
- expected_counts["audio"] > actual_counts["audio"]
- and self._sound_context_token_id is not None
- and token_id == self._sound_context_token_id
+ self._sound_context_token_id is not None
+ and token_id == self._sound_context_token_id
):
+ if actual_counts["audio"] >= expected_counts["audio"]:
+ return None
item_order.append(("audio", actual_counts["audio"]))
actual_counts["audio"] += 1
idx += 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tensorrt_llm/_torch/models/modeling_nemotron_nano.py` around lines 1576 -
1617, In _get_mm_item_order_from_token_ids detect and reject any MM placeholder
that appears after its expected count is exhausted: when matching the
multi-token video pattern (prompt_token_ids[idx:idx+video_pattern_len] ==
video_pattern) return None if actual_counts["video"] >=
expected_counts["video"]; similarly for single-token modalities (image using
self.img_context_token_id and audio using self._sound_context_token_id) if
token_id equals the placeholder but actual_counts[modality] >=
expected_counts[modality] return None immediately; this ensures extra
placeholders cause a fast fallback instead of being treated as plain text.
Comment on lines
+1402
to
+1424
| def _find_mm_embedding_lengths_from_masks( | ||
| mm_mask: torch.Tensor, | ||
| embed_mask: torch.Tensor, | ||
| num_mm_tokens: List[int], | ||
| ) -> List[int]: | ||
| """Compute per-item embedding-token counts from multimodal masks.""" | ||
| if not torch.any(mm_mask): | ||
| return [] | ||
|
|
||
| mm_positions = torch.where(mm_mask)[0] | ||
| lengths_t = torch.tensor(num_mm_tokens) | ||
| assert mm_positions.numel() == lengths_t.sum().item(), ( | ||
| f"Number of multimodal tokens ({mm_positions.numel()}) does not match " | ||
| f"sum of per-unit lengths ({lengths_t.sum().item()}): " | ||
| f"num_mm_tokens={num_mm_tokens}") | ||
|
|
||
| embedding_lengths: List[int] = [] | ||
| offset = 0 | ||
| for item_length in num_mm_tokens: | ||
| item_positions = mm_positions[offset:offset + item_length] | ||
| offset += item_length | ||
| embedding_lengths.append(int(embed_mask[item_positions].sum().item())) | ||
| return embedding_lengths |
Contributor
There was a problem hiding this comment.
Raise on mask/length mismatches instead of asserting.
This helper now feeds persisted multimodal_embedding_lengths. Under python -O, the assert disappears and a bad num_mm_tokens list can silently produce corrupted per-item lengths.
Suggested fix
mm_positions = torch.where(mm_mask)[0]
lengths_t = torch.tensor(num_mm_tokens)
- assert mm_positions.numel() == lengths_t.sum().item(), (
- f"Number of multimodal tokens ({mm_positions.numel()}) does not match "
- f"sum of per-unit lengths ({lengths_t.sum().item()}): "
- f"num_mm_tokens={num_mm_tokens}")
+ if mm_positions.numel() != lengths_t.sum().item():
+ raise ValueError(
+ f"Number of multimodal tokens ({mm_positions.numel()}) does not match "
+ f"sum of per-unit lengths ({lengths_t.sum().item()}): "
+ f"num_mm_tokens={num_mm_tokens}"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _find_mm_embedding_lengths_from_masks( | |
| mm_mask: torch.Tensor, | |
| embed_mask: torch.Tensor, | |
| num_mm_tokens: List[int], | |
| ) -> List[int]: | |
| """Compute per-item embedding-token counts from multimodal masks.""" | |
| if not torch.any(mm_mask): | |
| return [] | |
| mm_positions = torch.where(mm_mask)[0] | |
| lengths_t = torch.tensor(num_mm_tokens) | |
| assert mm_positions.numel() == lengths_t.sum().item(), ( | |
| f"Number of multimodal tokens ({mm_positions.numel()}) does not match " | |
| f"sum of per-unit lengths ({lengths_t.sum().item()}): " | |
| f"num_mm_tokens={num_mm_tokens}") | |
| embedding_lengths: List[int] = [] | |
| offset = 0 | |
| for item_length in num_mm_tokens: | |
| item_positions = mm_positions[offset:offset + item_length] | |
| offset += item_length | |
| embedding_lengths.append(int(embed_mask[item_positions].sum().item())) | |
| return embedding_lengths | |
| def _find_mm_embedding_lengths_from_masks( | |
| mm_mask: torch.Tensor, | |
| embed_mask: torch.Tensor, | |
| num_mm_tokens: List[int], | |
| ) -> List[int]: | |
| """Compute per-item embedding-token counts from multimodal masks.""" | |
| if not torch.any(mm_mask): | |
| return [] | |
| mm_positions = torch.where(mm_mask)[0] | |
| lengths_t = torch.tensor(num_mm_tokens) | |
| if mm_positions.numel() != lengths_t.sum().item(): | |
| raise ValueError( | |
| f"Number of multimodal tokens ({mm_positions.numel()}) does not match " | |
| f"sum of per-unit lengths ({lengths_t.sum().item()}): " | |
| f"num_mm_tokens={num_mm_tokens}" | |
| ) | |
| embedding_lengths: List[int] = [] | |
| offset = 0 | |
| for item_length in num_mm_tokens: | |
| item_positions = mm_positions[offset:offset + item_length] | |
| offset += item_length | |
| embedding_lengths.append(int(embed_mask[item_positions].sum().item())) | |
| return embedding_lengths |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tensorrt_llm/inputs/multimodal.py` around lines 1402 - 1424, The function
_find_mm_embedding_lengths_from_masks should raise an explicit exception when
the multimodal mask positions count doesn't match the sum of num_mm_tokens
instead of using an assert; replace the assert that checks mm_positions.numel()
== lengths_t.sum().item() with an if that raises a ValueError including
mm_positions.numel(), lengths_t.sum().item(), and the offending num_mm_tokens
list (use the same message text), leaving the early-return for empty mm_mask and
the subsequent computation using mm_positions, offset, and embed_mask unchanged.
Comment on lines
+587
to
+600
| for idx, (row, _default_subject) in enumerate(_iter_mmmu_rows(dataset_path)): | ||
| image = _get_mmmu_image(row) | ||
| if image is None: | ||
| continue | ||
| answer = _normalize_choice_answer(str(row.get("answer", "")), max_choice="E") | ||
| if answer is None: | ||
| continue | ||
| samples.append( | ||
| MixedModalityInputSample( | ||
| sample_id=str(row.get("id", row.get("sample_id", f"mmmu-{idx}"))), | ||
| media=image, | ||
| question=_format_mmmu_question(row), | ||
| keyword=answer, | ||
| source_paths=_get_media_source_paths(image), |
Contributor
There was a problem hiding this comment.
Don't score multi-image MMMU rows as single-image samples.
_format_mmmu_question(row) keeps the original MMMU question, but _get_mmmu_image() only feeds the first populated image* field into the sample. For rows that depend on comparing multiple images, this changes the task being evaluated and can make the mixed-vs-pure gate pass on a broken image target. Either preserve all referenced images or skip rows with more than one populated image field.
🛠️ Minimal safe fix
- image = _get_mmmu_image(row)
- if image is None:
+ images = [
+ image
+ for key, image in row.items()
+ if (key == "image" or key.startswith("image_")) and image is not None
+ ]
+ if len(images) != 1:
continue
+ image = images[0]
answer = _normalize_choice_answer(str(row.get("answer", "")), max_choice="E")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/defs/accuracy/mixed_modality.py` around lines 587 - 600,
The current loop uses _get_mmmu_image() which only returns the first populated
image field, causing multi-image comparison rows to be treated as single-image
samples; change the logic to detect rows with more than one populated image
field and skip them (safe minimal fix) or alternatively update _get_mmmu_image
to return all images and pass a list into MixedModalityInputSample. Concretely,
in the loop that calls _iter_mmmu_rows(dataset_path) and uses
_get_mmmu_image(row), count populated image* fields on the row (or modify
_get_mmmu_image to return a list and check its length); if more than one image
is present, continue (skip the row); otherwise proceed as before using the
single image, leaving _format_mmmu_question, MixedModalityInputSample, and
_get_media_source_paths unchanged.
| "audio": {"input_audio_features": "auds"}, | ||
| } | ||
| ) | ||
| with pytest.raises(KeyError, match="MixedModalItemOrder.*multimodal_item_order"): |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/unittest/_torch/modeling/test_nano_encode_extractor.py"
echo "== File exists? =="
test -f "$FILE" && echo "FOUND $FILE" || { echo "MISSING $FILE"; exit 1; }
echo "== Lines 300-312 =="
nl -ba "$FILE" | sed -n '300,312p'Repository: NVIDIA/TensorRT-LLM
Length of output: 230
🌐 Web query:
Ruff RUF043 match parameter non-raw string literal regex use raw string literal
💡 Result:
The Ruff lint rule RUF043, titled pytest-raises-ambiguous-pattern, identifies non-raw string literals passed to the match parameter of pytest.raises when those strings contain at least one unescaped regular expression metacharacter [1][2]. Why RUF043 exists: The match parameter in pytest.raises is implicitly treated as a regular expression [1][2]. When a standard (non-raw) string is used, ambiguity arises regarding whether the string is intended to be a literal match or a regular expression pattern [1][2]. Using a raw string (e.g., r"pattern") signals to readers and the interpreter that the string is intentionally a regular expression, preventing confusion caused by Python's standard string escaping rules [1][2][3][4]. How to resolve RUF043: - Use a raw string literal: If the string is intended to be a regular expression, prefix it with r (e.g., match=r"A pattern with. or *") [1][2]. - Escape metacharacters: If you intend to match literal characters that are also regex metacharacters (such as periods), escape them with backslashes (e.g., match="A literal dot\.") [1][2]. - Use re.escape: Alternatively, wrap the string in re.escape to automatically escape all characters that have special meaning in regex (e.g., match=re.escape("A literal dot.")) [1][2]. Ruff currently does not provide an automated fix for this rule because it cannot programmatically determine the user's intent—specifically, whether the string was intended to be a regex (requiring an 'r' prefix) or a literal match (requiring escaping or re.escape) [5][6].
Citations:
- 1: https://docs.astral.org.cn/ruff/rules/pytest-raises-ambiguous-pattern/
- 2: https://docs.astral.sh/ruff/rules/pytest-raises-ambiguous-pattern/
- 3: https://docs.python.org/3/library/re.html
- 4: https://docs.python.org/3/howto/regex.html
- 5: [
ruff] Stabilizepytest-raises-ambiguous-pattern(RUF043) astral-sh/ruff#16657 - 6: Consider adding a fix for
pytest-raises-ambiguous-pattern(RUF043) astral-sh/ruff#16690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/unittest/_torch/modeling/test_nano_encode_extractor.py"
awk 'NR>=300 && NR<=312 {print NR ":" $0}' "$FILE"Repository: NVIDIA/TensorRT-LLM
Length of output: 653
Use a raw regex literal in pytest.raises(..., match=...)
tests/unittest/_torch/modeling/test_nano_encode_extractor.py:308 trips Ruff RUF043 because match="MixedModalItemOrder.*multimodal_item_order" is a non-raw string containing regex metacharacters. Make it a raw string.
Suggested fix
- with pytest.raises(KeyError, match="MixedModalItemOrder.*multimodal_item_order"):
+ with pytest.raises(KeyError, match=r"MixedModalItemOrder.*multimodal_item_order"):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| with pytest.raises(KeyError, match="MixedModalItemOrder.*multimodal_item_order"): | |
| with pytest.raises(KeyError, match=r"MixedModalItemOrder.*multimodal_item_order"): |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 308-308: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unittest/_torch/modeling/test_nano_encode_extractor.py` at line 308,
The pytest.raises call using match="MixedModalItemOrder.*multimodal_item_order"
should use a raw regex string to avoid RUF043; update the test assertion (the
pytest.raises(...) call in test_nano_encode_extractor.py that checks for
KeyError) to use a raw string literal
(r"MixedModalItemOrder.*multimodal_item_order") so the
backslashes/metacharacters are interpreted as regex correctly.
Comment on lines
+2244
to
+2248
| # Per-slot embedding lengths keep the vision video + audio split: 256 | ||
| # video-context embeds (258 budget minus the 2 `<img>`/`</img>` specials) | ||
| # and 2 audio-context embeds (4 budget minus the 2 sound specials). | ||
| lengths = multimodal_data["multimodal_embedding_lengths"] | ||
| assert len(lengths) == 6 |
Contributor
There was a problem hiding this comment.
Assert the actual per-slot split, not just the slot count.
This regression test only checks len(multimodal_embedding_lengths) == 6, so a bug that preserves arity but merges or swaps the video/audio budgets would still pass. Given the comment already defines the expected split, tests/unittest/_torch/modeling/test_nemotron_nano_preprocessing.py coverage is currently insufficient here.
Suggested assertion
lengths = multimodal_data["multimodal_embedding_lengths"]
- assert len(lengths) == 6
+ assert lengths == [3, 256, 2, 3, 256, 2]As per coding guidelines, tests/** reviews should say whether coverage is sufficient; here coverage is insufficient because the test documents the exact split but does not pin it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unittest/_torch/modeling/test_nemotron_nano_preprocessing.py` around
lines 2244 - 2248, The test currently only asserts the length of
multimodal_data["multimodal_embedding_lengths"]; update it to assert the exact
per-slot split to prevent regressions (e.g., verify the list equals the expected
sequence described in the comment: video-context and audio-context budgets and
any slot-specific lengths). Locate the assertion around variable lengths
(multimodal_data["multimodal_embedding_lengths"]) in
test_nemotron_nano_preprocessing.py and replace or augment the len(...) check
with an assertion that the list's contents match the expected list of integers
(the explicit per-slot embedding lengths), so changes that reorder or merge
video/audio budgets will fail the test.
Comment on lines
+2671
to
+2826
| class TestVideoWithEmbeddedAudioHoist: | ||
| """Light guard that the REAL Nano processor hoists video-embedded audio into | ||
| a first-class top-level audio item, without model weights or a GPU. | ||
|
|
||
| `_prepare_video_modality_data` is vision-only (no nested audio); a single | ||
| video-with-audio request routes through `_process_mixed_modalities`, which | ||
| injects `<sound_context>`, promotes the audio to a standalone audio payload, | ||
| and emits a `(video, 0), (audio, 0)` prompt order. The per-item extractor | ||
| contract over the resulting payload is pinned in test_nano_encode_extractor.py. | ||
| """ | ||
|
|
||
| @staticmethod | ||
| def _make_proc(): | ||
| # Configure video_target_num_patches so _process_videos_frames uses the | ||
| # real video_to_pixel_values path (the HF image-processor fallback is a | ||
| # Mock here and is not subscriptable). | ||
| proc = _make_fast_path_audio_processor(max_num_patches=256, min_num_patches=4) | ||
| proc._add_video_prefix = False | ||
| proc.video_pruning_rate = 0 | ||
| proc.video_temporal_patch_size = 1 | ||
| proc.video_target_num_patches = 256 | ||
| proc.video_maintain_aspect_ratio = True | ||
| return proc | ||
|
|
||
| @staticmethod | ||
| def _video_with_audio(num_frames=2, num_audio_samples=16000): | ||
| frames = [Image.new("RGB", (512, 512)) for _ in range(num_frames)] | ||
| metadata = { | ||
| "total_num_frames": num_frames, | ||
| "fps": 30.0, | ||
| "duration": num_frames / 30.0, | ||
| "frames_indices": list(range(num_frames)), | ||
| } | ||
| audio = AudioData( | ||
| samples=np.random.randn(num_audio_samples).astype(np.float32), | ||
| sample_rate=16000, | ||
| ) | ||
| return SimpleNamespace(frames=frames, metadata=metadata, audio=audio) | ||
|
|
||
| def test_prepare_video_modality_data_is_vision_only(self): | ||
| # The video payload no longer nests audio — the audio is hoisted to a | ||
| # top-level item, so the video modality data carries only vision fields. | ||
| proc = self._make_proc() | ||
| video = self._video_with_audio() | ||
|
|
||
| modality_data = proc._prepare_video_modality_data([video]) | ||
|
|
||
| assert "pixel_values" in modality_data | ||
| assert "video_size" in modality_data | ||
| assert "audio" not in modality_data, "video payload must not nest audio (hoisted)" | ||
|
|
||
| def test_hoist_injects_sound_context_and_builds_audio_list(self): | ||
| # `_hoist_video_embedded_audio` injects `<sound_context>` after the video | ||
| # placeholder and extends mm_data["audio"] with the video's audio. | ||
| proc = self._make_proc() | ||
| video = self._video_with_audio() | ||
| mm_data = {"video": [video]} | ||
|
|
||
| hoisted_text, hoisted_mm_data = proc._hoist_video_embedded_audio("<video>", mm_data) | ||
|
|
||
| assert hoisted_text == "<video>" + AUDIO_PLACEHOLDER | ||
| assert len(hoisted_mm_data["audio"]) == 1 | ||
| # The original mm_data is not mutated. | ||
| assert "audio" not in mm_data | ||
|
|
||
| def test_no_hoist_when_video_has_none(self): | ||
| proc = self._make_proc() | ||
| video = self._video_with_audio() | ||
| video.audio = None | ||
| mm_data = {"video": [video]} | ||
|
|
||
| hoisted_text, hoisted_mm_data = proc._hoist_video_embedded_audio("<video>", mm_data) | ||
|
|
||
| assert hoisted_text == "<video>" | ||
| assert "audio" not in hoisted_mm_data | ||
|
|
||
| def test_video_embedded_audios_empty_without_extractor(self): | ||
| # The shared helper short-circuits to [] when no audio extractor is | ||
| # configured, so the promote/hoist early-outs both see "no audio". | ||
| proc = self._make_proc() | ||
| proc._audio_extractor = None | ||
| mm_data = {"video": [self._video_with_audio()]} | ||
|
|
||
| assert proc._video_embedded_audios(mm_data) == [] | ||
|
|
||
| def test_video_embedded_audios_returns_per_video_audio_or_none(self): | ||
| # With an extractor, the helper yields one entry per video: the embedded | ||
| # AudioData or None, preserving order. | ||
| proc = self._make_proc() | ||
| with_audio = self._video_with_audio() | ||
| without_audio = self._video_with_audio() | ||
| without_audio.audio = None | ||
| mm_data = {"video": [with_audio, without_audio]} | ||
|
|
||
| audios = proc._video_embedded_audios(mm_data) | ||
|
|
||
| assert len(audios) == 2 | ||
| assert audios[0] is with_audio.audio | ||
| assert audios[1] is None | ||
|
|
||
| def test_promote_is_idempotent(self): | ||
| # Re-promoting already-promoted data is a no-op: the hoisted video copies | ||
| # have `.audio` cleared, so the helper yields all-None and promote | ||
| # early-outs (returns the same object). | ||
| proc = self._make_proc() | ||
| mm_data = {"video": [self._video_with_audio()]} | ||
|
|
||
| promoted = proc.promote_nested_mm_data(mm_data) | ||
| assert proc._video_embedded_audios(promoted) == [None] | ||
|
|
||
| repromoted = proc.promote_nested_mm_data(promoted) | ||
| assert repromoted is promoted | ||
|
|
||
| def test_full_pipeline_emits_video_audio_plan(self): | ||
| # End-to-end through the REAL processor __call__ with the REAL hoist: a | ||
| # single video-with-audio yields a (video, 0), (audio, 0) prompt order | ||
| # and a top-level audio payload. The heavy per-modality prep + token | ||
| # ruler are mocked (orthogonal to the hoist + order scanning under test). | ||
| proc = self._make_proc() | ||
| proc._prepare_video_modality_data = mock.Mock( | ||
| return_value={"pixel_values": torch.ones(2, 3, 2, 2), "video_size": [[1, 1, 512, 512]]} | ||
| ) | ||
| proc._prepare_audio_modality_data = mock.Mock( | ||
| return_value={"input_audio_features": torch.ones(2, 4, 2)} | ||
| ) | ||
| # Per-slot budgets must equal the multimodal-token count the REAL | ||
| # `_expand_mixed_prompt_text_for_mm` emits (the only un-mocked count path | ||
| # here), so `compute_mm_embedding_lengths` reconciles. For this proc | ||
| # (`video_target_num_patches=256`, 2 frames) the video slot is 136 and the | ||
| # audio slot is 6. The video count is 132 vision tokens (2 tubelets x [64 | ||
| # context + 2 `<img>`/`</img>` specials]) plus 4 from this suite's Mock | ||
| # tokenizer encoding the long per-frame separator strings to id ranges | ||
| # that happen to include the context-token ids (a test-tokenizer artifact, | ||
| # not production behavior); the real `get_num_tokens_per_video` / | ||
| # `_compute_token_numbers_per_video` agreement is pinned separately by | ||
| # `test_get_num_tokens_per_video_agrees_with_compute_token_numbers_per_video`. | ||
| proc._get_num_tokens_for_item_order = mock.Mock(return_value=[136, 6]) | ||
|
|
||
| video = self._video_with_audio() | ||
| inputs = {"prompt": "<video>", "multi_modal_data": {"video": [video]}} | ||
|
|
||
| _, extra = proc(inputs, None) | ||
| multimodal_data = extra["multimodal_data"] | ||
|
|
||
| assert multimodal_data["modality_type"] == ["video", "audio"] | ||
| assert multimodal_data["multimodal_item_order"] == [ | ||
| {"modality": "video", "index": 0}, | ||
| {"modality": "audio", "index": 0}, | ||
| ] | ||
| # An audio payload was built from the hoisted video audio. | ||
| assert "audio" in multimodal_data | ||
| # The hoisted audio was prepared via the standalone audio path. | ||
| prepared_audios = proc._prepare_audio_modality_data.call_args[0][0] | ||
| assert len(prepared_audios) == 1 | ||
| # Two slots (video + audio) in the per-item embedding lengths. | ||
| assert len(multimodal_data["multimodal_embedding_lengths"]) == 2 |
Contributor
There was a problem hiding this comment.
Add the standalone+embedded-audio rejection case on the text path too.
TestTokenPathVideoEmbeddedAudioHoist now covers the ambiguous standalone audio + video-embedded audio combination, but TestVideoWithEmbeddedAudioHoist never exercises the same guard on _hoist_video_embedded_audio. That leaves tests/unittest/_torch/modeling/test_nemotron_nano_preprocessing.py with insufficient text-path coverage for a contract the token path already depends on.
As per coding guidelines, tests/** reviews should say whether coverage is sufficient; here coverage is insufficient because the text path is missing the rejection case that the token path already pins.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unittest/_torch/modeling/test_nemotron_nano_preprocessing.py` around
lines 2671 - 2826, Add a test to exercise the "standalone audio + video-embedded
audio" rejection on the text path: create a proc via
TestVideoWithEmbeddedAudioHoist._make_proc(), build mm_data containing a
top-level "audio" list (e.g., one AudioData) and a "video" list with a video
that has .audio set (use _video_with_audio()), then call
proc._hoist_video_embedded_audio("<video>", mm_data) and assert that it raises
(i.e., the hoist should reject the ambiguous combination). Reference the
existing test class and the methods _hoist_video_embedded_audio, _make_proc, and
_video_with_audio when adding this new test case.
Comment on lines
+496
to
+497
| for (modality, _), count in zip(order, num_mm_tokens_per_placeholder): | ||
| expanded.extend([feature_id[modality]] * count) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all zip() uses in this test file and confirm strict=True is present where paired lengths must match.
rg -nP 'zip\s*\(' tests/unittest/inputs/test_registry_mixed_multimodal.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 146
Add strict=True to avoid silent truncation in multimodal token re-expansion
At tests/unittest/inputs/test_registry_mixed_multimodal.py line 496, zip(order, num_mm_tokens_per_placeholder) can silently truncate if lengths ever diverge; adding strict=True makes the test fail loudly instead.
Suggested fix
- for (modality, _), count in zip(order, num_mm_tokens_per_placeholder):
+ for (modality, _), count in zip(order, num_mm_tokens_per_placeholder, strict=True):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (modality, _), count in zip(order, num_mm_tokens_per_placeholder): | |
| expanded.extend([feature_id[modality]] * count) | |
| for (modality, _), count in zip(order, num_mm_tokens_per_placeholder, strict=True): | |
| expanded.extend([feature_id[modality]] * count) |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 496-496: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unittest/inputs/test_registry_mixed_multimodal.py` around lines 496 -
497, The zip in the loop that builds expanded tokens uses zip(order,
num_mm_tokens_per_placeholder) which can silently truncate mismatched lengths;
update the loop in tests/unittest/inputs/test_registry_mixed_multimodal.py where
the comprehension/loop "for (modality, _), count in zip(order,
num_mm_tokens_per_placeholder):" appears to call zip(..., strict=True) so the
test will raise if the two iterables differ in length, making mismatches fail
loudly.
…-tile actuals The Nemotron Nano token-id fast path (call_with_token_ids) baked per-image embedding lengths at the full _max_num_patches cap, while the encoder tiles the image at the reservation-reduced budget. For a budget-squeezed dynamic image in a mixed image+audio request the two diverged, so emitted encoder rows no longer matched the baked lengths and the encoder_dispatch cross-check rejected the request with a ValueError. The text path already reconciled this; the token-id path did not. Add a model-overridable hook, reconcile_dynamic_image_token_counts, that splices the dummy-pass post-tile actuals into the planned per-image counts before placeholder expansion and length baking. Nano implements it (mirroring the text path's splice); the generic call_with_token_ids dispatches it optionally so the base class stays model-agnostic. Move the existing video-embedded-audio hoist hook off the base the same way, and rename both hooks to describe their actual behavior (hoist_video_embedded_audio, reconcile_dynamic_image_token_counts). Also document build_scatter_index's destination-row index math with a worked example (comments only). Signed-off-by: venkywonka <23023424+venkywonka@users.noreply.github.com>
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
3 participants
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.
What
Gate
Validation
FP8 video was the only pure-only delta: -6.25 target points, within the 10.0 point per-target gate. All aggregate and per-target mixed-vs-pure gates passed.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests