fix: TRT-LLM multimodal preprocessor - remove default_multimodal_input_loader from the embedding paths#6924
Conversation
|
👋 Hi moraxu! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
62d4bbb to
61ff322
Compare
WalkthroughAdds optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/src/dynamo/trtllm/multimodal_processor.py`:
- Line 326: The current logging line logs the entire processed_inputs (including
raw prompt text and tensors) which can leak sensitive prompts and create huge
logs; change the logging in the block that emits logging.info(f"Processed
inputs: {processed_inputs}") to instead log a sanitized summary: redact or omit
the prompt text and only log safe metadata such as tensor shapes/dtypes and
masked prompt length (or a boolean indicating presence of prompt), or call a
sanitizer function (e.g., sanitize_processed_inputs) before logging; ensure you
update the logging level to debug if needed and retain enough info for debugging
without printing full prompt content or tensor data.
In `@lib/llm/src/preprocessor.rs`:
- Around line 1278-1281: Formatting is failing around the call to
self.gather_multi_modal_data in preprocessor.rs; run rustfmt (cargo fmt) to
format the file and commit the updated formatting so the call site and
surrounding block conform to rustfmt rules (e.g., adjust spacing/indentation
around the builder invocation and the await? operator). Ensure the formatted
changes that include the gather_multi_modal_data(&request, &mut builder,
None).await? line are committed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 486b9c66-0b40-4d2e-bca7-fa7976520e2b
📒 Files selected for processing (2)
components/src/dynamo/trtllm/multimodal_processor.pylib/llm/src/preprocessor.rs
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
61ff322 to
3ec32fa
Compare
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
indrajit96
left a comment
There was a problem hiding this comment.
@rmccorm4
@KrishnanPrash
@grahamking
For rust side changes
@moraxu worker changes LGTM except minor changes
Ran my local tests ALL PASS
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
|
/ok to test a8e8d25 |
…t_loader from the embedding paths (#6924) Signed-off-by: Michal Guzek <mguzek@nvidia.com> Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Overview:
A follow up to #6840 to remove
default_multimodal_input_loadercalls from the embedding paths and instead pass txt prompt from the Rust frontend (where the chat template has already been applied to it) to TRT-LLM.Details:
Where should the reviewer start?
Tested
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Refactor