Repository navigation
Feat/madlad trt rtx fp16 - #421
Conversation
MadladTokenizerDecoder.EncodeSourceText built the encoder input from the
SentencePiece ids alone (the tokenizer is created with
addEndOfSentence: false) and never appended the end-of-sentence token.
T5-family encoders are trained on inputs closed by </s>, so greedy
decoding ran past the sentence: it repeated it ("Bonjour, comment
allez-vous aujourd'hui ? Bonjour, comment allez-vous aujourd'hui ?"),
appended paraphrases, or invented clauses. The OPUS-MT source tokenizer
already adds EOS.
On a fixed 12-sentence en->fr fixture against madlad400-3b-mt, outputs
go from 5 of 12 repeated or hallucinated to 12 clean single
translations, and decoding gets faster because outputs stop at the
real sentence end.
Adds a fixture-gated regression test (TRACKDUB_MADLAD_FIXTURE_ROOT).
Trackdub.Inference.Tests net10.0: 1057 passed, 39 skipped, 0 failed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add a trt-fp16 MADLAD variant (the hosted mixed fp16/fp32 encoder and plain decoder, pinned to c165d741) that only lists trt-rtx, and plan MADLAD on TensorRT RTX first, then CPU (int4 KV-cache), then DirectML. TensorRT RTX JIT-compiles kernels for every new input shape, so on that provider the engine pads encoder input to length buckets, re-runs the decoder prefix padded to a bucket, and reads the logits at the last real position. Causal self-attention and the encoder mask keep the padding out, so the output matches unpadded fp16 decoding (pinned by a fixture test). The TRT RTX sessions replay a CUDA graph per bucket, and the no-cache decoder now fetches only logits instead of copying every present.* cache back each step. The encoder-decoder session constructs and runs under the TRT RTX EP ABI plugin on both the ORT 1.24 Windows ML root and ORT 1.30, so MADLAD leaves the fatal TensorRT smoke guard (opus-mt stays). A TRT RTX session failure falls back to CPU unless the provider was hard-pinned. Warm one-segment clip, translation stage, RTX 5070: 2.1 s on TensorRT RTX vs 7.9 s for int4 on CPU. The sessions hold ~8 GB of VRAM. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TRT RTX sessions rebuilt every engine from the source graph in each new process; the runtime cache only keeps JIT kernels. EP-context artifacts were produced only by the manual warm pass, and broke for models whose engines exceed protobuf's 2 GB limit. - A TRT RTX session for a graph of at least 256 MB without a current artifact now builds with EP-context generation on and publishes the artifact next to the model, so the next process deserializes engines instead of building them. The capture reuses the build the session performs anyway. It is skipped when the volume lacks room, and a capture failure retries the session plainly. - Engines too large to embed are published as separate files. The stamp records their names and sizes, and a missing or truncated engine invalidates the artifact. Replacing an artifact removes the previous stamp first, then its recorded files. - The embed decision and the stamp now count Optimum's <model>.onnx_data external weights, not only <model>.onnx.data. - The non-pooled encoder-decoder factories (used by smoke probes) go through CreateSession, so they load artifacts instead of rebuilding from source. MADLAD-400 3B fp16 on an RTX 5070, fresh process, translation stage: 83.4 s before, 29.6-40.5 s after. That includes a smoke-probe pair and the pooled pair; each EP-context load takes 5-13 s for the encoder and 7-21 s for the decoder, against ~50 s or more to build each from source. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 46 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No identified issue prevents merging after normal checks. Security Architecture Review
Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The required template sections are present, but the authored description leaves the summary, linked issue, test notes, architecture review, license/model impact, risk and rollback, milestone notes, an… | Complete the required sections. Add a concise summary, linked issue or explicit justification, actual build and test results, test notes, architecture and model-impact confirmations, risk and rollback details, milestone notes, and agent not… |
| Docstring Coverage | |
Docstring coverage is 17.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 91 functions across 19 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
-
-
Check name Status Explanation Title check The title clearly identifies the main change: adding MADLAD TensorRT RTX FP16 support. It is concise and related to the changeset. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
-
Full details: Description check
-
-
Explanation
The required template sections are present, but the authored description leaves the summary, linked issue, test notes, architecture review, license/model impact, risk and rollback, milestone notes, and agent notes largely unfilled. The checklist does not report build or test results.
Resolution
Complete the required sections. Add a concise summary, linked issue or explicit justification, actual build and test results, test notes, architecture and model-impact confirmations, risk and rollback details, milestone notes, and agent notes. Mark checklist items accurately.
-
Full details: Docstring Coverage
-
-
Explanation
Docstring coverage is 17.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 91 functions across 19 files. (1 skipped: 1 unsupported.)
-
✨ Finishing Touches 💡 1
-
-
📝 Generate docstrings 💡
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
🧪 Generate unit tests (beta)
-
-
- Commit to this branch
- Create a new PR
-
✨ Simplify code
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
PR Summary by QodoEnable MADLAD TensorRT RTX translation and reusable engine artifacts
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?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: 3519a58a3d
ℹ️ 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".
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (3) 🔗 Fix PR: #422 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 3 fixed
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
|
Review submitted successfully. Here is the summary: Review: pullrequestreview-5477924627 This PR enables MADLAD translation on TensorRT RTX through a new
Finding 1 — Finding 2 — |
|
No noteworthy issues found. This increment (review_base
The one candidate I investigated in depth (the rollback catch block's lack of per-item exception handling) was dispositioned as self-healing: the narrow TOCTOU window requires a reader to open the EP-context model between the publisher's file move and stamp write, and even then the consequence is orphaned disk (no stamp → rebuild), not data corruption. Not actionable. |
|
Review submitted: pullrequestreview-5478042447 This increment (review_base One new finding:
|
|
I've completed a thorough review of this increment (review_base I traced the full publication/rollback flow, verified the No noteworthy issues found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
main now carries #419 (Windows ML 2.4 plus the MADLAD KV-cache work this branch builds on). Conflicts were the shared MADLAD files; this branch's versions are kept and main's later changes are re-applied on top: the GenAI provider list without DirectML, kokoro on CUDA, the quantized MADLAD variant as the default with its exports out of the model-level download list, and the updated MADLAD fallback comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Snapshot validation passed. Since the PR was force-pushed and the review_base (ca3910c) is from a different commit tree, the preflight cannot anchor inline comments against the PR diff. Per the skill, I'm returning a concise markdown summary with both confirmed findings. Review: Increment This increment introduces an out-of-process ORT 1.30 inference worker, GenAI native runtime selection with SHA256 provenance, DirectML runtime probing, TTS streaming prefetch, and a Windows ML 2.4.89 package upgrade. Two confirmed findings; inline comment submission was rejected by the anchor preflight (the pinned diff comparison spans two disjoint commit trees due to the force-push), so both are reported here.
Finding 1 — Finding 2 — Out of diff: No additional summary-only findings. The InferenceWorkerClient lifecycle, TtsStreamingPrefetch concurrency, provider discovery/scoping, and protocol wire format were all investigated and found correct. |
…ction - GenAiNativeRuntimeSelection.LoadedPath returns null when the process cannot list its modules (Win32Exception on restricted Windows), so SelectPath falls back to the on-disk candidate instead of failing native loading. The post-load identity check still fails closed. - The inference worker creates its Kokoro engine lazily without caching a construction failure, so one transient error no longer fails every later TTS request for the worker's lifetime. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review: increment The two prior findings from the last review (LoadedPath Win32Exception bypass, Lazy<KokoroTtsEngine> faulted-state caching) are both resolved:
Finding 1 — Finding 2 — When Finding 3 — Finding 4 — Excluding DirectML from GenAI families causes Qwen2.5-1.5B-Instruct to fall back to CPU on Windows, but the model's |
Summary
Linked issue
Scope
Testing
dotnet build Trackdub.slnx -m:1dotnet test Trackdub.slnx -m:1Test notes
Architecture review
Trackdub.AppLicense/model impact
Risk and rollback
Milestone notes
Agent notes
Summary by cubic
MADLAD, previously excluded from TensorRT RTX, now runs on it through a new
trt-fp16model variant with a static-shape greedy decoder that pads inputs to length buckets so the JIT compiles kernels once per bucket. First-time engine builds for large graphs are captured as EP-context artifacts, so later processes deserialize engines instead of rebuilding them.</s>, fixing MADLAD outputs that repeated, paraphrased, or invented clauses.RunWithRetrycan fetch only named outputs, so the no-cache decoder stops copying everypresent.*cache back each step.Written for commit 0d9afa0. Summary will update on new commits.
Summary by CodeRabbit