tensorrt_llm/_torch/models/modeling_phi4mm.py (6)

36-42: Special token IDs should be documented more clearly

The special token constants reference HF source files but could benefit from documenting their semantic meaning. Also, the float('-inf') usage in line 41 is unusual for a token ID range.

Consider adding more descriptive comments and verifying the float range intention:

-# Special token ids from the original Phi-4-multimodal-instruct implementation
-_IMAGE_SPECIAL_TOKEN_ID = 200010  # '<|endoftext10|>' from HF `modeling_phi4mm.py`
-_AUDIO_SPECIAL_TOKEN_ID = 200011  # '<|endoftext11|>' from HF `modeling_phi4mm.py`
-_PAD_TOKEN_ID = 199999  # '<|endoftext|>' from HF `special_tokens_map.json`
-_COMPATIBLE_IMAGE_SPECIAL_TOKEN_ID_RANGE = [-9999,
-                                            -1]  # from HF `modeling_phi4mm.py`
-_COMPATIBLE_AUDIO_SPECIAL_TOKEN_ID_RANGE = [float('-inf'), -10000
-                                            ]  # from HF `modeling_phi4mm.py`
+# Special token ids from the original Phi-4-multimodal-instruct implementation
+# These tokens are used to mark placeholder positions for multimodal content
+_IMAGE_SPECIAL_TOKEN_ID = 200010  # '<|endoftext10|>' marks image position
+_AUDIO_SPECIAL_TOKEN_ID = 200011  # '<|endoftext11|>' marks audio position
+_PAD_TOKEN_ID = 199999  # '<|endoftext|>' padding token from HF `special_tokens_map.json`
+# Compatible ranges for legacy token IDs that may appear in older checkpoints
+_COMPATIBLE_IMAGE_SPECIAL_TOKEN_ID_RANGE = [-9999, -1]  # from HF `modeling_phi4mm.py`
+# Note: Using sys.float_info.min would be more explicit than float('-inf')
+_COMPATIBLE_AUDIO_SPECIAL_TOKEN_ID_RANGE = [-2147483648, -10000]  # from HF `modeling_phi4mm.py`

---

383-389: Environment variable control flow should be centralized

The PHI4_MM_PER_REQUEST_INFER environment variable check duplicates the pattern seen with TLLM_MULTIMODAL_DISAGGREGATED. Consider centralizing environment variable handling.

Create a centralized configuration helper:

+def _use_per_request_inference() -> bool:
+    """Check if per-request inference mode is enabled."""
+    return os.getenv("PHI4_MM_PER_REQUEST_INFER", "0") == "1"
+
 @torch.inference_mode()
 def forward(self, multimodal_params: List[MultimodalParams],
             mm_token_ids: torch.Tensor) -> List[torch.FloatTensor]:
-    if os.getenv("PHI4_MM_PER_REQUEST_INFER", "0") == "1":
+    if _use_per_request_inference():
         # Reference code path to check correctness of batch inference and further dev.
         # (TODO) Remove this path after accuracy bench and data parallelism are supported.
         return self._encoding_per_request(multimodal_params, mm_token_ids)
     else:
         # Batch inference as default path.
         return self._encoding_batch_request(multimodal_params, mm_token_ids)

---

399-401: Error message should be more informative

The ValueError for trust_remote_code provides insufficient context about why it's required.

 if not trust_remote_code:
-    raise ValueError("trust_remote_code must be True for Phi4MM")
+    raise ValueError(
+        "trust_remote_code must be True for Phi4MM to load the required "
+        "model-specific components from the Hugging Face repository"
+    )

---

482-485: Improve disaggregated mode error message

The error message could be more helpful by indicating when support might be available or what alternatives exist.

 if _is_disagg():
     raise ValueError(
-        "Phi4MM does not support disaggregated inference yet.")
+        "Phi4MM does not support disaggregated inference yet. "
+        "Please use AGGREGATE mode by setting TLLM_MULTIMODAL_DISAGGREGATED=0"
+    )

---

163-265: Consider memory optimization for batched inference

The batch inference methods create zero-initialized tensors for padding (lines 187-193, 237-244). For large batch sizes or high-resolution images, this could consume significant memory.

Consider these optimizations:

1. Use sparse representations when padding ratio is high
2. Pre-allocate and reuse buffers across inference calls
3. Add memory profiling to track peak usage

Would you like me to suggest a memory-efficient implementation using buffer pooling?

---

267-332: The _encoding_per_request method has redundant tensor operations

The method processes each request individually, which could be inefficient for batches. Also, the tensor masking operations (lines 291-292, 328-329) are performed per request.

Consider caching common operations or moving invariant computations outside the loop. The current implementation is correct but could be optimized when this becomes the production path.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: nvpohanh
PR: NVIDIA/TensorRT-LLM#7478
File: tensorrt_llm/_torch/models/modeling_llama.py:1315-1315
Timestamp: 2025-09-03T13:16:06.824Z
Learning: The Llama4VisionEncoder.load_weights method signature is `def load_weights(self, weights: Dict)` and should not be confused with Llama4ForConditionalGeneration.load_weights which has a different signature including weight_mapper parameter.