Repository navigation
fix(native): improve backend diagnostics and context handling - #1158
justinchuby wants to merge 1 commit into
Conversation
87ad108 to
4a7c011
Compare
Propagate pipeline context metadata, distinguish the ORT support library from backend selection, and report native CUDA whole-session fallback explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
|
Rebased again onto latest main after its sibling PRs landed and re-resolved the four conflict hunks in pipeline/mod.rs. All four are independent additions -- generation_defaults (from #1162) and max_sequence_length (this PR) touching the same struct field list, accessor block, �uild locals, and constructor -- so the resolution is to keep both on every hunk. The accessor hunk needed a closing brace inserted, since the two sides had been sharing one. Re-validated after the rebase: cargo test -p onnx-genai-engine --no-default-features --features native-backend --test pipeline_executor -> 2 passed, including pipeline_exposes_declared_context_limit. The reviewer had already falsified that test (dropping the .or(self.max_sequence_length) reddens it with left: None, right: Some(16)). Merging. |
4a7c011 to
b2f295b
Compare
#1158) (#1200) Same content as #1158, reopened on a branch based directly on `main`. #1158 was reviewed, rebased and validated, but could not be merged: it is the base of #1161, and GitHub refuses both `gh pr merge` and a base change for stacked pull requests, directing callers to an asynchronous merge endpoint this repository does not expose (404). Rather than leave two reviewed PRs stuck behind an API we cannot reach, the commit is cherry-picked onto `main` here and #1158 is closed. #1161 is then free to rebase onto `main` normally. The rebase produced four conflicts in `pipeline/mod.rs`, all the same shape as the earlier ones in this family: this branch adds `max_sequence_length` and `effective_max_context` while `main` has since added `generation_defaults`. They are independent additions in the same struct, constructor and accessor block, so both sides are kept; the accessor hunk needed a closing brace reinserted because the two sides shared one. Validation on the rebased tree: `pipeline_executor` 2 passed, and the engine lib suite 508 passed / 0 failed / 1 ignored, matching the baseline on `main`. Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
|
Closing in favour of #1200, which carries this exact commit on a branch based directly on main. This PR could not be merged: it is the base of #1161, and GitHub blocks both merge and base changes for stacked pull requests, pointing at an asynchronous merge endpoint this repository returns 404 for. The content was reviewed and validated, so it is not being dropped -- only re-landed off the stack, which also unblocks #1161. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1158 +/- ##
==========================================
+ Coverage 79.60% 79.85% +0.25%
==========================================
Files 357 359 +2
Lines 153790 157326 +3536
Branches 153790 157326 +3536
==========================================
+ Hits 122419 125631 +3212
- Misses 26850 27135 +285
- Partials 4521 4560 +39
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison 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
|
…as silence (#1214) Unstacked replacement for #1161, which GitHub will not let us merge: it was stacked on #1158, and the stack metadata survives even though that PR is now closed and its content is in main via #1200. `gh pr merge`, `gh pr edit --base`, and both async merge endpoints all refuse or 404. Same branch content, off `main`, no stack. ## What it does Formats structured model responses in the CLI, and fixes a silent-empty-reply bug found while reviewing it. `load_response_config` succeeds from `tokenizer_config.json` alone, but the marker strings are only resolved to token ids later, against the tokenizer. If a template names a marker the tokenizer does not encode as a single special token, `message_token_id` stays `None` -- and `ResponseStream` starts in `Header`, whose only exit is `message_token_id == Some(token.token_id)`. `None` never equals `Some(_)`, so the parser is trapped in a state where every output carries `include_in_reply: false`. **The caller gets an empty string and no error: a good generation reported as silence.** The fix withdraws structured mode entirely rather than degrading in place. Raw output shows the markers as literal text -- a poorer presentation, but an honest one. Silence is neither. It is a mechanism, not a patch: `bind_response_tokens` is the single guarded entry point, and `bind_token_ids` is now module-private so no future caller can bind without the check. Only `message_token_id` is treated as load-bearing; withdrawing on the optional markers would drop structured mode for templates that parse fine. ## Validation - `onnx-genai-cli` lib: **121 passed / 0 failed** - engine lib: **508 / 0 / 1**, matching main's baseline - Both new tests falsified: forcing `is_parseable` to `true` turns them RED on the exact assertion they claim to guard - The two `repl_e2e` attachment failures (`missing required pipeline input 'encoder.pixel_values'`) reproduce identically on unmodified main and are unrelated to this branch Supersedes #1161. --------- Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Summary
Validation