Repository navigation
Recognize a missing pipeline input by the message the pipeline writes - #1413
Merged
Merged
Conversation
Sending a prompt that references an image without attaching one used to answer with advice: this model wants an image, attach it with `/image`. That advice went missing and the REPL surfaced the raw `missing required pipeline input 'encoder.pixel_values'` instead. The front end recognized the condition by matching the string `"input not found: "`, which is what a single decoder graph raises. A pipeline raises its own wording, so once a multimodal package started failing through the pipeline router the match no longer fired. Its unit test did not catch this because it hand-wrote the same string it was testing against, so both sides agreed with each other and neither agreed with the engine. Name the message once, next to where the pipeline writes it, and export the recognizer from the engine. A front end now asks the engine whether an error is a missing input rather than guessing at its prose, and the message and its recognizer cannot drift apart because they share the constant. The test moves to the engine, where it also pins the case that must *not* be advised as a forgotten attachment: an optional input that a presence key declared present is a package-authoring fault. The REPL end-to-end tests that cover this were failing on main and pass again. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
✅ Benchmarks — No RegressionComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1413 +/- ##
===========================================
+ Coverage 80.88% 82.99% +2.10%
===========================================
Files 364 12 -352
Lines 160729 5638 -155091
Branches 160729 5638 -155091
===========================================
- Hits 130005 4679 -125326
+ Misses 26069 761 -25308
+ Partials 4655 198 -4457
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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
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.
Symptom
Sending a prompt that references an image without attaching one answered with the raw engine error:
instead of the advice the REPL is supposed to give ("this model declares image input; attach one with
/image <path>"). Tworepl_e2etests were failing onmainbecause of it.Cause
The front end recognized the condition by matching
"input not found: "— the wording a single decoder graph raises. A pipeline raises its own wording fromrouting.rs, so once a multimodal package started failing through the pipeline router the match stopped firing.The unit test guarding this hand-wrote the same string it tested against, so the test and the matcher agreed with each other while neither agreed with the engine. That is why the drift was invisible.
Fix
Name the message once, next to where the pipeline writes it (
MISSING_REQUIRED_INPUT), and export the recognizer (is_missing_required_input) from the engine. A front end now asks the engine whether an error is a missing input rather than guessing at its prose, and the message and its recognizer cannot drift apart because they share the constant.Tests
The test moves to the engine, next to the constant, and additionally pins the case that must not be advised as a forgotten attachment: an optional input that a presence key declared present is a package-authoring fault, not a missing file.
cargo test -p onnx-genai-cli -p onnx-genai-engine— the two previously failingrepl_e2etests pass (47/47). clippy and fmt clean.Note:
decode_position_and_state_e2e::pipeline_load_rejects_kv_page_pool_before_fixed_state_when_host_budget_cannot_fit_eitheralso fails onmain, from an unrelated change to how the KV budget resolves under a tiny host limit. It is not touched here.