[TRTLLM-11272][fix] Account for the existing multimodal placeholder tokens in a text prompt#12827
Conversation
📝 WalkthroughWalkthroughA utility function in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tensorrt_llm/inputs/utils.py (1)
548-553: Add regression tests for the new subtraction behavior.The logic change looks correct, but please add targeted tests for:
existing == count,existing > count, and mixed placeholder keys in one prompt to lock this behavior in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tensorrt_llm/inputs/utils.py` around lines 548 - 553, Add unit tests for the placeholder-subtraction logic around mm_placeholder_counts and text_prompt: create tests exercising (1) existing == count (no placeholders added), (2) existing > count (no negative additions and no removals), and (3) a mixed prompt containing multiple different placeholder keys to ensure combined counts are handled correctly; each test should call the function that builds placeholders (referencing mm_placeholder_counts, text_prompt, and placeholders) and assert the returned text_prompt or resulting placeholders list matches the expected behavior for those three scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tensorrt_llm/inputs/utils.py`:
- Around line 548-553: Add unit tests for the placeholder-subtraction logic
around mm_placeholder_counts and text_prompt: create tests exercising (1)
existing == count (no placeholders added), (2) existing > count (no negative
additions and no removals), and (3) a mixed prompt containing multiple different
placeholder keys to ensure combined counts are handled correctly; each test
should call the function that builds placeholders (referencing
mm_placeholder_counts, text_prompt, and placeholders) and assert the returned
text_prompt or resulting placeholders list matches the expected behavior for
those three scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c150b1c-49b2-4703-a774-7c369c61c04e
📒 Files selected for processing (1)
tensorrt_llm/inputs/utils.py
|
/bot run |
|
PR_Github #42376 [ run ] triggered by Bot. Commit: |
2ez4bz
left a comment
2ez4bz
left a comment
There was a problem hiding this comment.
Can you add a unit test for the scenario this fixes?
|
PR_Github #42376 [ run ] completed with state
|
|
/bot run |
|
PR_Github #42395 [ run ] triggered by Bot. Commit: |
|
/bot run |
|
PR_Github #42413 [ run ] triggered by Bot. Commit: |
|
PR_Github #42413 [ run ] completed with state
|
|
/bot run |
2 similar comments
|
/bot run |
|
/bot run |
|
PR_Github #42588 [ run ] triggered by Bot. Commit: |
|
PR_Github #42588 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
moraxu
force-pushed
the
dev-mguzek-account-for-existing-placeholder-tokens
branch
from
88bd14c to
a23ad14
Compare
|
/bot run |
1 similar comment
|
/bot run |
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
moraxu
force-pushed
the
dev-mguzek-account-for-existing-placeholder-tokens
branch
from
a23ad14 to
40c0669
Compare
|
/bot run |
2 similar comments
|
/bot run |
|
/bot run |
|
/bot run --disable-fail-fast |
|
PR_Github #43965 [ run ] triggered by Bot. Commit: |
|
PR_Github #43965 [ run ] completed with state
|
|
/bot run |
4 similar comments
|
/bot run |
|
/bot run |
|
/bot run |
|
/bot run |
|
PR_Github #44073 [ run ] triggered by Bot. Commit: |
|
PR_Github #44073 [ run ] completed with state
|
|
/bot run |
|
PR_Github #44135 [ run ] triggered by Bot. Commit: |
|
PR_Github #44135 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #44145 [ run ] triggered by Bot. Commit: |
|
PR_Github #44145 [ run ] completed with state
|
|
/bot run |
|
PR_Github #44152 [ run ] triggered by Bot. Commit: |
|
PR_Github #44152 [ run ] completed with state
|
|
/bot run |
|
PR_Github #44165 [ run ] triggered by Bot. Commit: |
|
PR_Github #44165 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #44223 [ run ] triggered by Bot. Commit: |
|
PR_Github #44223 [ run ] completed with state
|
|
/bot run |
|
PR_Github #44268 [ run ] triggered by Bot. Commit: |
|
PR_Github #44268 [ run ] completed with state |
|
|
5 participants
Summary by CodeRabbit
Description
Account for the existing multimodal placeholder tokens in a text prompt
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.