fix(inkling): align multimodal placeholders with checkpoint template - #1931
Conversation
github-actions
Bot
added
tokenizer
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughInkling now emits dedicated image and audio placeholder tokens with updated feature ranges. Chat templates can invoke a registered ChangesInkling placeholder handling
Template error reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the InklingSpec multimodal registry to use unused placeholder tokens for image and audio modalities, adjusting the prompt replacement logic and tests accordingly. It also registers a raise_exception helper function in the chat template's MiniJinja environment to surface model-authored validation errors. Feedback suggests using minijinja::Value instead of String for the exception message parameter to avoid type coercion failures when non-string values are passed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn raise_exception(message: String) -> std::result::Result<String, MinijinjaError> { | ||
| Err(MinijinjaError::new(ErrorKind::InvalidOperation, message)) | ||
| } |
There was a problem hiding this comment.
Using String as the parameter type for raise_exception forces MiniJinja to attempt type coercion. If a template passes a non-string value (such as a boolean, number, or complex object) to raise_exception, MiniJinja will fail with a type coercion error (e.g., invalid type: ..., expected a string) instead of raising the actual authored validation message.
Using minijinja::Value instead allows the function to accept any type and convert it to a string representation via .to_string(), ensuring the authored validation message is always preserved and surfaced.
| fn raise_exception(message: String) -> std::result::Result<String, MinijinjaError> { | |
| Err(MinijinjaError::new(ErrorKind::InvalidOperation, message)) | |
| } | |
| fn raise_exception(message: Value) -> std::result::Result<String, MinijinjaError> { | |
| Err(MinijinjaError::new(ErrorKind::InvalidOperation, message.to_string())) | |
| } |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean and well-tested change. The placeholder realignment correctly delegates structural markers to the checkpoint template and expands only the soft placeholders, with structural_prefix(1) properly accounting for the leading marker. The raise_exception addition is a standard HF compatibility function. No issues found.
Signed-off-by: chenht2022 <chenht2022@gmail.com>
c8ce6b8 to
2075db7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2075db730e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Modality::Image => Ok(Self::IMAGE_PLACEHOLDER.to_string()), | ||
| Modality::Audio => Ok(Self::AUDIO_PLACEHOLDER.to_string()), |
There was a problem hiding this comment.
When an Inkling deployment uses a String-format chat template, the rendering path replaces image_url/input_audio parts with the value returned here before the checkpoint template has a chance to add TML markers. Returning the soft placeholders means those prompts contain only <|unused_...|>, while the replacement code still assumes a preceding <|content_image|>/<|content_audio_input|> via with_structural_prefix(1) and does not re-emit it; expansion then folds the previous text/newline token into the structural range and the final prompt is missing the typed marker. The OpenAI-format checkpoint template may be fine, but the supported String-template path regresses unless the content marker remains the rendered anchor or is emitted in the replacement.
Useful? React with 👍 / 👎.
2 participants
Description
Problem
The current Inkling multimodal path expands inputs from the structural image/audio markers.
The checkpoint-provided chat template emits each marker followed by one soft placeholder:
<|unused_200054|><|unused_200053|>Expanding the marker leaves the template-emitted soft placeholder in the final prompt as an extra token. The template also uses
raise_exception(...)for input validation, which is not currently registered in SMG's MiniJinja environment.Solution
Expand the checkpoint-provided soft placeholders directly and keep the preceding image/audio marker in the structural range through
structural_prefix(1).Also register the Hugging Face-compatible
raise_exceptionhelper so template validation errors preserve their authored messages.Changes
200054.200053.<|audio_end|>.raise_exceptionin the chat-template environment.Test Plan
cargo test -p llm-multimodal inklingcargo test -p llm-tokenizer test_raise_exception_surfaces_template_validation_messagecargo clippy -p llm-multimodal -p llm-tokenizer --all-targets -- -D warningscargo +nightly fmt --all -- --checkpre-commit run --all-filesgit diff --checkChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Bug Fixes
Error Handling