Repository navigation
PITCH: Rust inference sidecar (decision-only) - #394
Conversation
Decision-only proposal: load-once/serve-forever sidecar seam to dodge Python cold start without breaking single-installer distribution. Nothing in src/ changes; sketches are not wired into any build.
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. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This is a well-documented decision-only pitch for a Rust inference sidecar. The proposed architecture is clear, and the PR correctly isolates all changes to docs/ and tools/ without impacting the build system.
Critical Finding
One logic error was identified in the sketch code (main.rs) where inference could proceed without a loaded model due to an incorrect readiness check. A fix has been suggested.
Overall Assessment
The pitch is viable for spike approval once the logic defect is addressed. The IPC contract design is sound, and the phased approach (Rust first, Python behind same contract if needed) preserves the distribution story while addressing cold-start concerns.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| }) | ||
| } | ||
| "infer" => { | ||
| if session.is_none() && active_provider == "none" { |
There was a problem hiding this comment.
🛑 Logic Error: Inference can proceed with no loaded model. The condition checks session.is_none() && active_provider == "none", but lines 44-45 set active_provider to a provider name while leaving session = None. This allows inference requests to pass the check and return "ok" status without an actual model loaded, causing silent failures.
| if session.is_none() && active_provider == "none" { | |
| if session.is_none() { |
There was a problem hiding this comment.
Fixed (same as above): guard is now session.is_none() only, and the load stub can no longer report loaded without a committed session.
tonythethompson
left a comment
tonythethompson
left a comment
There was a problem hiding this comment.
Reviewed the pitch and the spike sketches. Five scoped threads: two on the decision itself (what Phase 1 actually buys, and the single-binary claim), one on the JSON-lines contract, and two small ones in the Rust sketch.
| - **Phase 1 (this pitch):** a Rust ONNX sidecar (`tools/rust-sidecar-spike/`) | ||
| that owns one job — load an ONNX graph once, serve inference over IPC with | ||
| ms-scale warm calls. It proves the seam: C# stage → IPC contract → sidecar | ||
| process lifecycle (start, health, model-ready, infer, shutdown). |
There was a problem hiding this comment.
Phase 1 doesn't address either problem in section 1. The Rust sidecar is ONNX-only (table row "Runs .pt-only models: no"), and ONNX models already run in-process in C# with resident sessions, so there's no process start or per-call weight load to save. The cold-start table in section 5 compares the sidecar against spawning Python per call, but that's the baseline for .pt models, which this phase can't run. Against the real Phase 1 baseline (in-process ORT), the sidecar adds IPC serialization, a second process, and a second binary to sign.
Ask: either state Phase 1's value plainly as "prove the out-of-process seam and contract that Phase 2 needs" and benchmark it against in-process ORT, or reorder the decision so the .pt question (Q4, and the third checkbox in section 7) is answered first, since it decides whether the seam is needed at all.
There was a problem hiding this comment.
Addressed in bdee843/b4e5fa3e: Section 3 now states Phase 1's value plainly as seam-proving (not speedup), Section 5 re-baselines against in-process ORT, and Section 7 leads with the .pt-need question first.
| | | Python resident daemon | Go sidecar | TypeScript sidecar | **Rust sidecar (pitched)** | | ||
| |---|---|---|---|---| | ||
| | Cold start | ~1–5 s first load, ms after (resident) | ms (static binary) | 100s of ms + runtime | ms (static binary) | | ||
| | Ships as | 2–4 GB env, CUDA-matched | single binary | needs Node/Bun + native addons | single binary | |
There was a problem hiding this comment.
"Single binary" probably doesn't hold once GPU providers are in. With the ort crate, CPU can be statically linked, but GPU execution providers (DirectML, CUDA/TensorRT-RTX) generally load as shared libraries that have to ship next to the executable. That means the sidecar carries its own ONNX Runtime and provider payload alongside the copy the C# app already ships.
Two ORT copies also opens version skew: an EP context or engine cache built by one ORT version may not be valid for the other, which touches the same fingerprint discipline the doc leans on in section 5. Worth a row in the table (or an open question) for distribution size and ORT version pinning across the two processes.
There was a problem hiding this comment.
Addressed: table corrected to colocated provider libs, new ORT-copies/version-skew row, and new open question Q6 on cross-process ORT pinning, fingerprints, and signing.
| // C# → sidecar | ||
| { "id": "req-1", "op": "load", "model": "models/kokoro-onnx/model.onnx", | ||
| "providers": ["TensorRTRTX", "DirectML", "CPU"] } | ||
| { "id": "req-2", "op": "infer", "inputs": { "input_ids": [101, 234, 999] } } |
There was a problem hiding this comment.
The JSON-lines infer request has no dtype or shape. input_ids: [101, 234, 999] can't tell the sidecar whether that's int64[1,3] or int32[3], and multi-dimensional inputs (mel features, speaker embeddings) don't fit a flat array at all. The proto sketch already carries TensorMeta { shape, dtype }, so as written the spike contract and the graduation contract differ, which undercuts "survives a move to gRPC without redesign."
Ask: give the JSON-lines tensors the same shape ({ "dtype": "int64", "shape": [1, 3], "data": "<base64>" }) so the spike exercises the real contract. Base64 in JSON also costs about 33% on PCM outputs, which is worth measuring in the spike before picking the transport.
There was a problem hiding this comment.
Addressed: spike contract now uses {dtype, shape, data} envelopes matching TensorMeta on both paths, and the ~33% base64 tax is called out as an explicit spike measurement.
| }) | ||
| } | ||
| "infer" => { | ||
| if session.is_none() && active_provider == "none" { |
There was a problem hiding this comment.
This guard lets infer return ok with no model loaded. After any load, active_provider is set to the first preference while session stays None, so session.is_none() && active_provider == "none" is false and the else branch replies status: ok. That's the "GPU ready when it wasn't" pattern the pitch is trying to rule out, and it's the kind of placeholder that survives into the real spike. Keying only on the session keeps the readiness states honest even while load is stubbed:
| if session.is_none() && active_provider == "none" { | |
| if session.is_none() { |
There was a problem hiding this comment.
Fixed: readiness keys only on the committed session; the load stub now answers unimplemented rather than claiming a provider (b4e5fa3).
| // Real code: build ort::value::Value inputs from | ||
| // req["inputs"], session.run(...), base64 the PCM bytes. | ||
| let _inputs: HashMap<String, serde_json::Value> = serde_json::from_value( | ||
| req["inputs"].clone().unwrap_or_default(), |
There was a problem hiding this comment.
This line won't compile: serde_json::Value has no unwrap_or_default(). req["inputs"].clone() is already a Value (it's Null when missing), and the trailing .unwrap_or_default() on the from_value result covers that case. Since Cargo.toml makes this a real crate, the first cargo build in the spike would trip on it.
| req["inputs"].clone().unwrap_or_default(), | |
| req["inputs"].clone(), |
There was a problem hiding this comment.
Fixed: dropped the invalid unwrap_or_default on serde_json::Value.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The sketches currently fake readiness, bypass planner guarantees, omit required tensor metadata, and propose a Python phase that conflicts with repository policy.
Review effort: Balanced
Findings: 3
Open (7)
What changed in this PR
Proposes a decision-only Rust inference sidecar architecture without build integration.
Changes:
- Documents the phased Rust/Python sidecar proposal.
- Sketches JSON-lines and gRPC contracts.
- Adds illustrative Rust server and C# client code.
| File | Description |
|---|---|
docs/plans/rust-inference-sidecar-pitch.md |
Architecture pitch and decision criteria |
tools/rust-sidecar-spike/Cargo.toml |
Sketch Rust dependencies |
tools/rust-sidecar-spike/src/main.rs |
JSON-lines server sketch |
tools/rust-sidecar-spike/proto/inference.proto |
Proposed gRPC contract |
tools/rust-sidecar-spike/csharp/SidecarClient.sketch.cs |
Proposed C# integration seam |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Task<SidecarLoadResult> LoadAsync( | ||
| string modelPath, IReadOnlyList<string> providers, CancellationToken ct); |
There was a problem hiding this comment.
Addressed in b4e5fa3: the seam now takes a planner-produced SidecarLoadPlan (mirrors StageRuntimePlan, hard-pin honored) and the doc states the sidecar executes but never selects, so planner gates cannot be bypassed.
| let mut stdout = std::io::stdout(); | ||
|
|
||
| for line in stdin.lock().lines() { | ||
| let req: serde_json::Value = serde_json::from_str(&line?)?; |
There was a problem hiding this comment.
Fixed: malformed lines now yield an invalid-json protocol error and the loop keeps serving.
| active_provider = providers.into_iter().next().unwrap_or("CPU".into()); | ||
| session = None; // placeholder: real session goes here | ||
| let _ = &model; | ||
| serde_json::json!({ | ||
| "id": id, "status": "loaded", | ||
| "activeProvider": active_provider, "loadMs": 0, | ||
| }) |
There was a problem hiding this comment.
Fixed: the load stub answers load-not-implemented-in-sketch and only a committed session may ever report loaded; infer keys solely on the session.
| Task<SidecarInferResult> InferAsync( | ||
| string id, IReadOnlyDictionary<string, byte[]> inputs, CancellationToken ct); |
There was a problem hiding this comment.
Addressed: C# sketch now carries SidecarTensor (dtype, shape, data) on both request and result paths, matching the proto TensorMeta envelope.
| // Real code: build ort::value::Value inputs from | ||
| // req["inputs"], session.run(...), base64 the PCM bytes. | ||
| let _inputs: HashMap<String, serde_json::Value> = serde_json::from_value( | ||
| req["inputs"].clone().unwrap_or_default(), |
There was a problem hiding this comment.
Fixed in an earlier push (same invalid unwrap_or_default removal).
| So: we need a sidecar story that keeps cold start in the milliseconds, ships as | ||
| a single binary, and doesn't reopen the distribution wound. |
There was a problem hiding this comment.
Addressed: process-start vs first-inference split in the table and a terminology note in Section 5; the loadMs example is labeled illustrative with the real number an explicit spike measurement.
| - **Phase 2 (only if needed):** a Python resident daemon implementing the | ||
| *same* IPC contract, used exclusively for `.pt`-only frontier voice models | ||
| (Chatterbox/CosyVoice) where no usable ONNX exists. Same readiness states, | ||
| same artifact story — just a different executor the user opts into. |
There was a problem hiding this comment.
Addressed and agreed: pitch scope narrowed to the Rust spike only. Phase 2 is now explicitly blocked on a governance change plus an experimental packaging boundary, not pre-approved here.
…sketch fixes
- Restate Phase 1 value as seam-proving (not speedup); re-baseline the
cold-start table against in-process ORT; lead decision checkboxes
with the .pt-need question (Q4)
- Correct single-binary claim: colocated ORT/provider libs, new version
skew row, new Q6 on ORT pinning/fingerprints/signing
- Typed tensor envelope ({dtype, shape, data}) in spike contract to
match TensorMeta; call out base64 tax as spike measurement
- Fix readiness guard to key only on session; drop invalid
unwrap_or_default on serde_json::Value
…ernance scope - Sidecar executes planner-approved plans (SidecarLoadPlan mirrors StageRuntimePlan); never selects models/providers itself - Rust sketch: malformed JSON yields invalid-json and keeps serving; load stub answers unimplemented instead of fake loaded - C# sketch carries typed SidecarTensor payloads both directions - Doc: split process-start vs first-inference; Phase 2 explicitly deferred pending governance + packaging decision, not pre-approved
|
Decision recorded (2026-10-06, Tony): Python is inevitable for This push updates the pitch: new Section 5b lays out the Rust supervision design (pinned bundled interpreter, lockfile, resident warmed worker, supervisor-enforced readiness, crash containment with backoff, same IPC contract, telemetry from day one). Q4 and the first decision checkbox are marked decided; remaining asks are the Phase 1 spike approval and the supervised-worker direction under a scoped AGENTS.md exception (ADR to follow). Rule of the design: Rust supervises, Python computes, C# orchestrates. |
|
ADR and spike plan are in: ADR-0016 (scoped Python exception, mirrored into decisions.md) plus docs/plans/rust-sidecar-spike-plan.md — two-week time-box, tools-only, five exit criteria (seam proven, tax measured, contract holds across both executors, distribution sized, failures demonstrated), then a go/no-go review before anything touches src/. Still draft until you say otherwise. |
…lan (#401) * docs: record decision — Python inevitable, contained via Rust supervision - Status accepted-direction; new Section 5b supervision design (pinned interpreter, lockfile, resident warm worker, enforced readiness, crash containment, same IPC contract, telemetry) - Q4 + first checkbox marked decided; Phase 2 re-scoped as Rust-supervised worker under a forthcoming ADR exception * docs: ADR-0016 supervised Python worker exception + sidecar spike plan - Records 2026-10-06 decision: scoped end-user Python exception, conditional on Rust supervision (pitch Section 5b design) - Spike plan: two-week time-box, tools-only, five exit criteria, go/no-go review gate before any src/ work * docs: rewrite sidecar pitch as committed build plan, drop the hedging - Status decided (ADR-0016); Phase 1+2 both committed and sequenced - Section 6 questions converted to decided-or-spike-owned items - Section 7 decision checklist converted to execution checklist; reject option removed * docs: land sidecar decisions — ADR-0016, spike plan, unhedged build plan Follow-up to #394, which merged the hedged revision: records the accepted direction (Rust-supervised Python worker), the two-week spike box, and the rewritten plan without deferred decisions. * Add atomic translation revision and take publication Introduce an atomic publication boundary for staged translation revisions and generated TTS takes. `AtomicRevisionCommitBoundary` owns acceptance of streaming translation output (retaining envelopes for validation at commit time) and take inputs, delegating to `SqliteAtomicRevisionPersistence`, which uses `BEGIN IMMEDIATE` write transactions to validate expected source/translation heads, allocate revision numbers, insert rows, and publish artifacts under a single writer reservation. Concurrent writers cause head-conflict failures rather than retries against newer inputs. Artifact publication becomes create-only: `IArtifactStore.CommitNewAsync` never overwrites, and an `ArtifactPromotionReceipt` records attempt ownership, enabling recovery logic (fresh write reservation, durable-row checks, checksum-verified deletion of owned unreferenced files) after commit ambiguity, cancellation, or failures. Translation artifacts are prepared and fingerprinted outside the write lock. Stale-take marking is scoped to changed indices (text, timing, or source hash) within the same language lineage, preserving unchanged, unlinked, and other-language takes. Readiness adds a `bundle-needed` resolve action for automatic model selections vs `download` for explicit ones, backed by `RuntimeModelSelectionIntent` provenance and `StudioSettings.AutomaticModelAliases`. Adds integration tests (`AtomicRevisionCommitBoundaryTests`, `ReadinessResolveActionTests`) plus a unit-test persistence boundary shared by application tests. * Revert "Add atomic translation revision and take publication" This reverts commit 5d9f2f6. * Fix review findings: AGENTS.md carve-out, scope consistency, audit trail - AGENTS.md Model Governance names the ADR-0016 sole exception - Spike plan anchored to PR #401; controlled-matrix lane moved to promotion (keeps tools-only scope honest); criterion 4 gains three-OS sizing + signing measurement - Pitch binary claim scoped to supervisor; ADR decided-by line corrected here and in the decisions.md mirror
What this is
A decision-only pitch: a load-once/serve-forever Rust sidecar seam to dodge Python cold start on frontier voice models without breaking the single-installer distribution story. Follow-up to the PyTorch-vs-ONNX and TensorRT-RTX discussions.
What's in it (5 files, none wired into any build)
docs/plans/rust-inference-sidecar-pitch.md— the pitch: problem, options table (Python daemon vs Go vs TS vs Rust), recommendation, IPC sketch, open questionstools/rust-sidecar-spike/— code sketches: Rust server loop (src/main.rs),Cargo.toml, gRPC graduation contract (proto/inference.proto), C# caller sketch (.sketch.cs, never compiles)What's explicitly NOT in it
src/changes, no.slnxchanges, no CI impact (verified: no slnx referencestools/)Recommendation in brief
Rust ONNX sidecar first (ms cold start, single binary, first-class ORT bindings); Python resident daemon later behind the same IPC contract, only for
.pt-only models with no usable ONNX. C# + in-process ORT stays the default path.Decision requested (see doc section 6–7)
.pt-only models?Test plan
git diff --statshows only docs/tools additions)