Conversation
Adds GpuInfo.IsRtxSpark, additive only. Groundwork for routing RTX Spark's unified-memory SKUs through a fixed recipe table instead of the generic capacity fit-test.
|
/review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. Reviewed September 25, 2026, 7:30 PM ET / 23:30 UTC (Revision 26). ClawSweeper reviewWhat this changesThe branch adds fixed RTX Spark model recommendations, verified downloads for a separate DFlash draft model, Local AI receipt support for that asset, setup and tray changes, and a newer pinned llama-server runtime. Merge readiness⛔ Blocked before merge - 5 items remain Current main and the latest release still use generic model selection for RTX Spark. A patch-equivalent maintainer replacement is open at #1520, but the change has not landed on main and this repository’s close policy does not permit closing this PR yet. The prior 32 GB setup-copy finding also remains. Priority: P2 Review scores
Verification
How this fits togetherWindows Local AI reads GPU facts to choose a model and runtime. Setup downloads pinned files and records an installation receipt, which the runtime verifies before launching llama-server. flowchart LR
A[GPU facts] --> B[Model selection]
B --> C[Fixed Spark recipe or generic choice]
C --> D[Setup downloads]
D --> E[Installation receipt]
E --> F[File verification]
F --> G[Local inference]
Decision needed
Why: The new receipt is intentionally unreadable to older builds; source and upgrade proof establish forward compatibility, but only maintainers can accept the downgrade behavior and its user-facing recovery obligation. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land one Spark recipe branch after giving the 32 GB no-default result accurate localized setup and tray copy and recording the older-build recovery contract; keep split-GGUF support in #1508. Do we have a high-confidence way to reproduce the issue? Not applicable as a feature request. Current main’s generic selection is source-verifiable, and the linked hardware traces exercise the proposed Spark path. Is this the best way to solve the issue? Mostly yes: a separate Spark selector preserves the discrete-GPU path, and pinned asset verification supports DFlash. The 32 GB diagnostic and downgrade contract still need resolution. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 5a59535216ee. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (25 earlier review cycles; latest 8 shown)
|
13f474d to
b9c116b
Compare
clawsweeper
Bot
added
merge-risk: 🚨 availability
b9c116b to
f94cf90
Compare
Real behavior proof: RTX Spark SKU routing on real hardwareCaptured from the live WinUI setup flow on a real 48 GB RTX Spark at current head (
This shows the full changed path working end to end:
ValidationReal 48 GB RTX Spark (arm64 runtime, CUDA 13), current head,
Every review finding from the previous passes is fixed and squashed into the commit it belongs to: GPU binding for Spark recipes, retired-runtime launch compatibility across the b10655 bump, hash verification of additional model assets before native load, and preservation of the SKU profile when setup round-trips the recommended model id. Each fix ships with a regression test, and each test was confirmed to fail without its fix. |
|
@clawsweeper re-review Real behavior proof has been added: a live WinUI capture from a real 48 GB RTX Spark at current head showing Spark detection, the SKU-selected recipe, its pinned 96K context profile, and the download disclosure. The PR body and proof-pool declarations are updated to match. All four prior review findings are fixed and squashed into their originating commits, each with a regression test confirmed to fail without its fix. |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
the
proof: 📸 screenshot
f94cf90 to
e6ce446
Compare
|
@clawsweeper re-review [P1] Keep existing runtime receipts upgradeable during setup - fixed and squashed into Confirmed against the source: The receipt is now validated against the runtime it actually recorded ( Regression added: and passes with it, returning ValidationReal 48 GB RTX Spark, current head
12 commits, largest 374 lines. Every review finding to date is fixed and squashed into its originating commit, each with a regression test confirmed to fail without its fix. |
|
🦞👀 Re-review progress:
|
|
@clawsweeper re-review Real-path traces captured on a real 48 GB RTX Spark at current head 1. Multi-asset acquisition and native servingInstalled the DFlash recipe, whose draft checkpoint is a second pinned asset in a different Hugging Face repository than the primary weights. This exercises the whole added path end to end: schema-5 receipt written with the draft checkpoint recorded at its pinned size and digest, the asset resolved into the shared hub cache from its own repository and revision, 2. Existing pre-bump install upgradingTook the completed install above, rewrote its receipt to the retired b10655 runtime as a pre-bump install would appear on disk, and reran setup. The b10655 receipt is retained as the upgrade baseline rather than ending setup, the new runtime is installed, the verified model and its draft checkpoint are reused instead of being downloaded again, and the receipt is replaced in place at b11026. ValidationReal 48 GB RTX Spark, head
CI unit suites are green at this head. The remaining |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
@shanselman @karkarl — this is ready for a merge decision at head What it doesRoutes RTX Spark unified-memory devices through NVIDIA's fixed SKU table instead of the generic capacity fit-test, in a parallel decision layer that leaves the dGPU path untouched. 12 commits, no fixup commits — every finding from every review cycle is squashed into the commit it belongs to, each with a regression test verified to fail without its fix. Karen's gates, dischargedYour triage is pinned to head
A re-triage at Scott's P1, fixed
I also confirmed the Change Model route you asked about: Next steps
CompatibilityThe CI and follow-upsEvery unit, integration and validation job passes at this head, including Required local validation at this head on a real 48 GB RTX Spark: The 96 GB split-GGUF recipe was removed rather than shipped in a state that would either fail to load or weaken hub-cache file identity; that work is tracked in #1508. |
|
@clawsweeper re-review — head is unchanged at Still outstanding from your side: the P2 you set at 09:33:20Z has no published finding text — no review, no inline comment, nothing on the PR that states what it is. Please include it in this pass so it can be acted on. |
|
🦞👀 Re-review progress:
|
shanselman
added
the
status: 🚢 actively landing
clawsweeper
Bot
added
the
merge-risk: 🚨 availability
|
Maintainer closeout at exact head Blocking source finding: I reproduced this with a temporary ordering regression: initial runtime snapshot had no model, delayed runtime refresh returned a valid managed receipt, and the gated hardware probe completed afterward. The current code failed Validation: The required |
shanselman
removed
the
status: 🚢 actively landing
clawsweeper
Bot
added
P1
bkudiess
added
status: 🚢 actively landing
RTX Spark's total CUDA-visible memory identifies the physical memory SKU, not usable capacity the way it does on a discrete GPU, so the generic priority/fit-test default pick doesn't apply. Adds: - SpeculativeDecodingMode.None/DraftDFlash and an optional separate draft checkpoint (LocalModelRunRecipe.DraftWeights), needed because DFlash uses an independently pinned draft GGUF rather than an embedded MTP draft layer. - Three RTX Spark catalog recipes: Qwen3.6-35B-A3B IQ4_XS (48GB SKU), and Qwen3.8-27B Q4_K_M with DFlash n=7 (128GB SKU default). The 64GB-SKU recipe reuses the existing default model's ctx-131072-q8_0 profile -- no new catalog entry needed. - RtxSparkInferenceSelector, a fixed SKU-to-recipe table keyed off GpuVisibleMemoryBytes (empirically verified against a real 48GB RTX Spark unit). Wired into LocalInferenceSelector.Select only for the no-requested-model default path; explicit model requests and every non-Spark GPU keep using the untouched generic fit-test. - GetRequiredMemoryBytes/GetDraftKvCacheMemoryBytes now account for a DFlash recipe's separate draft weights and skip draft KV entirely for SpeculativeDecodingMode.None.
BuildPreset hardcoded spec-type = draft-mtp unconditionally. Branches on LocalModelRunRecipe.SpeculativeDecoding so DraftDFlash recipes emit spec-type = draft-dflash plus spec-draft-model, and None emits no spec-* lines at all. DraftDFlash still requires a resolved draftModelPath; that path isn't threaded from the install manifest yet (the DFlash draft checkpoint isn't acquired/verified through HuggingFaceModelInstaller in this change), so BuildPreset fails closed with InvalidDataException rather than silently omitting the draft model. Follow-up work.
Adds SKU-boundary coverage for the new RtxSparkInferenceSelector (32/48/64/128GB) and a case proving a non-Spark GPU with Spark-sized memory still takes the generic path. Evaluate_RoutesRuntimeByArchitectureWithoutGpuSkuPairing used "NVIDIA RTX Spark N1X" as an arbitrary placeholder name to prove GPU name doesn't affect runtime routing. That's no longer SKU-irrelevant now that RTX Spark has real SKU routing, so the fixture GPU name is swapped to a generic dGPU.
LocalModelCatalog.AdditionalArtifacts() gives every acquirer, manifest, and launch path a single fixed ordering for a recipe's non-primary pinned artifacts. Today that is the DFlash draft checkpoint. GetRequiredMemoryBytes now includes the draft checkpoint's weights, so a DFlash recipe does not under-report the memory it needs during qualification.
b10655 (CUDA 13.3 on x64) predates DFlash draft-decoding support and the 96GB recipe's validated build. Bumping to b11026 puts both x64 and arm64 on CUDA 13.4 and covers every RTX Spark recipe with one pinned runtime, matching the existing single-global-pin design instead of adding per-recipe runtime routing.
…sets Extends LocalAiInstallManifest with AdditionalModelAssets/ AdditionalModelPaths (schema 5) so a recipe's DFlash draft checkpoint or extra split-GGUF shards can be recorded and re-verified alongside the primary weights receipt in the same Hugging Face hub cache. Schema 4 manifests are untouched -- the new fields are empty and absent from JSON unless a recipe actually pins additional assets. Each additional-asset receipt derives its own repository/revision from its own SourceUrl (not the primary ModelId) since the DFlash draft checkpoint is pinned from a different HF repo than the primary weights. Adds LocalAiInstallManifest.UsesHubCache so schema 4 and schema 5 are treated identically everywhere the manifest previously branched only on the exact HubCacheReceiptSchemaVersion value. AdditionalModelAssets/AdditionalModelPaths are left at their unset ImmutableArray default (not .Empty) so JsonIgnoreCondition.WhenWritingDefault actually omits them for schema-3/4 manifests -- .Empty is a distinct, non-default array instance the condition never matches, so writing it would have added new fields to every existing schema-4 receipt and broken older app builds' strict unknown-field rejection. UsesHubCache is marked [JsonIgnore] for the same reason: it's a derived read helper, not part of the persisted contract. ResolveAndValidate normalizes the unset default to .Empty immediately after load so every in-memory reader keeps using ordinary IsEmpty/Length calls safely.
Adds HuggingFaceModelInstaller.InstallAdditionalAssetAsync, mirroring InstallAsync's resumable-download/verify/promote flow for a recipe's non-primary pinned artifacts (DFlash draft checkpoint, extra split-GGUF shards). Deliberately duplicated rather than refactored out of InstallAsync to avoid any risk to that heavily-tested primary weights path; the one thing it omits is the legacy app-owned compatibility copy, since every recipe using an additional asset is new since the hub cache became the primary store.
…ncile AcquireLocalAiModelStep now downloads/verifies a recipe's additional assets (via LocalModelCatalog.AdditionalArtifacts) right after its primary weights, records the results on SetupContext, and rolls the context field back on failure -- the hub-cache artifacts themselves survive rollback the same way the primary weights' do, since there is no legacy copy to delete. PersistLocalAiManifestStep writes them into a schema-5 manifest (schema 4 unchanged for recipes with none), and leaves the two new manifest fields at their unset default rather than an explicitly-built empty array when there is nothing to add, so a plain schema-4 install keeps omitting them from JSON. LocalAiInstallReconciler gains VerifyAdditionalAssetAsync so a reused install re-verifies a recipe's additional assets against the hub cache, not just its primary weights, before treating the install as still valid. LocalAiReconcileResult now also carries the additional asset installs it just verified, reconstructed from the manifest's own already-verified receipts: recovery for a broken runtime (model and additional assets still valid) previously left SetupContext's additional-install list empty because AcquireLocalAiModelStep's reuse skip never re-runs acquisition, which made PersistLocalAiManifestStep hard-fail on the very installs this series adds an acquisition step for. The setup review consent screen now also lists every additional artifact (a DFlash draft checkpoint, or extra split-GGUF shards) as its own download line instead of only the primary weights.
…unch preset BuildCore resolves the DFlash draft checkpoint and passes it to BuildPreset, so spec-draft-model is finally emitted for real. ValidateArtifactReceipts also checks additional-asset receipts against the catalog, as it already does for the primary weights and runtime artifacts. The path handed to llama-server is the handle-resolved physical path from the same verification that opened the file, not the persisted snapshot path. The hub cache hands out a handle-resolved path precisely so a snapshot-link replacement cannot change the file identity a native reader finally opens, which is how the primary model is already bound; the draft checkpoint now gets the same guarantee instead of re-deriving its path from its receipt. LlamaServerRuntimeService's schema checks use LocalAiInstallManifest.UsesHubCache so schema-5 installs are treated as hub-cache-backed, matching schema 4, instead of falling into the legacy schema-3 branch.
A recipe's pinned weights are not everything it downloads or loads: a DFlash recipe also pulls a separate draft checkpoint, and llama-server loads both. Ranking, the post-launch GPU-load sanity check, and the user-facing size disclosure all read the primary weights alone, so a DFlash recipe understated its footprint and the setup review omitted the draft checkpoint entirely. Adds LocalModelCatalog.TotalDownloadSizeBytes (weights plus draft checkpoint) and switches SelectDefaultModelAndProfile's tie-break and fallback, LocalAiGpuVerification's minimum-load-delta check, and the setup UI's model picker and detail text to use it. SelectDefaultModelAndProfile also excludes priority-0, explicit-alternative models from the generic default and fallback pool, so a model reachable only through a specific SKU can never win the generic pick as the catalog grows.
…are incomplete An RTX Spark GPU whose CUDA memory couldn't be read reads as 0 bytes, which the fixed SKU table treated as a legitimately-too-small SKU (NotRecommendedForSku) instead of the real problem: unreadable capacity. Select() now only takes the Spark SKU-routing branch when the Spark GPU's facts are actually complete, so an incomplete-facts Spark GPU falls through to the generic path and gets correctly diagnosed as HardwareFactsIncomplete, same as any other GPU. Also bumps LocalAiPortHandoffTests' Spark fixture from an arbitrary ~24GiB (a pre-SKU-routing placeholder, now below the smallest 32GB tier) to a real 48GB SKU's measured cuMemGetInfo total, so it again resolves to the 24GB recipe instead of "not recommended."
362e728 to
24ff075
Compare
|
@shanselman — the ordering race is fixed and pushed as Fix, in string? previousModelId = _runtimeSnapshot.ModelId;
_runtimeSnapshot = snapshot;
OnPropertyChanged(null);
if (IsActive && !string.Equals(previousModelId, snapshot.ModelId, StringComparison.Ordinal))
StartAvailabilityRefresh();That is the single funnel every snapshot passes through — both Permanent regression: Proven against your requirement, reverting only the ViewModel and keeping the test:
Both changes are squashed into the commits they belong to — the fix into Validation at this head on a real 48 GB RTX Spark: Your point about the existing hardware proof still applying holds for this head too — the delta since |
|
@clawsweeper re-review — head is now |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
removed
the
P1
|
Maintainer replacement #1520 was created from a clean rebase of this exact 12-commit patch stack onto current main. The external contributor branch was not force-pushed. git range-diff reports all 12 commits patch-equivalent (=); replacement head is 7e51993, includes #1515, and carries updated validation/proof plus the current-head DGX rerun blocker. Do not merge both PRs. |
This branch has not been deployed
4 participants
Closes #1496
Routes RTX Spark unified-memory SKUs through NVIDIA's fixed recipe table instead of
the generic capacity fit-test, and extends the catalog, manifest, and installer so the
recipes NVIDIA validated can actually be expressed and acquired.
Discrete GPU machines keep their existing behavior. The documented "GPU names and
CPU/GPU SKU pairings are not part of qualification" invariant stays true for every
non-Spark device: the Spark path is a separate decision layer, and an explicitly
requested model still uses the same capacity fit-test everywhere.
What changed
through
RtxSparkInferenceSelectorusing geometric-midpoint SKU bands.SpeculativeDecodingMode.NoneandDraftDFlash, plus an optional separatelypinned
LocalModelRunRecipe.DraftWeights(DFlash's draft checkpoint lives in adifferent Hugging Face repository than the target weights).
AdditionalModelAssets/AdditionalModelPaths) so extrasplit-GGUF shards and the draft checkpoint are recorded and re-verified in the same
hub cache. Schema 4 receipts are written and serialized exactly as before.
HuggingFaceModelInstaller.InstallAdditionalAssetAsyncacquires those assets withthe same resumable download, hash verification, and promotion flow as the primary
weights.
BuildPresetbranches on the recipe's speculative decoding mode and emits a realspec-draft-modelpath for DFlash.Required proof pools
windows-wsl-dgx-blackwell: RTX Spark SKU detection, recipe routing, multi-asset acquisition, draft file identity at native load, native llama-server serving, and pre-bump install upgrade. Validated on a real 48 GB RTX Spark (arm64) and a native x64 RTX 5090.windows-winui-interactive: the Local AI model picker size text and the setup review download list changed. Captured live on a real 48 GB RTX Spark; screenshot in the PR comments.Validation
Real 48 GB RTX Spark (arm64 runtime, CUDA 13), current head, repository-required closeout:
./build.ps1succeeded with exit code 0 ("All builds succeeded"), including proof-pool anddocumentation-flow validation.
OpenClaw.Tray.TestsOpenClaw.Shared.TestsOpenClaw.Connection.TestsOpenClaw.SetupEngine.TestsCI is green on every unit and validation job at this head. The remaining
Revocation recovery E2E,Setup and connect E2E,Network recovery E2E, andCI Gatefailures reproduceidentically on
mainand are independent of this branch.Real behavior proof
Ground truth for the SKU boundary came from the real device rather than an assumption.
A direct
nvcuda.dllprobe (cuInit/cuDeviceGetCount/cuMemGetInfo_v2) on the48 GB RTX Spark reports a total of
48,585,498,624bytes, which is whatCudaHostHardwareProbereads and what the 48 GB band is anchored to.nvidia-smireports 24,512 MiB on the same device via NVML, which is the under-report that #1422
already established cannot be trusted here.
Changed UI strings, rendered from the real catalog at current head on the RTX Spark
box:
Every pre-existing recipe renders byte-identical to before. Only the DFlash recipe
changes, which is the defect this fixes: its draft checkpoint was not disclosed at all.
The 96 GB Qwen3.8 Flash-Next recipe is not part of this PR. Split-GGUF loading needs the
shard filenames while the hub cache's integrity model supplies a content-addressed path,
and reconciling the two needs install-flow surface that belongs in its own change. Tracked
in #1508.
Multi-asset acquisition and native serving are proven on the device. Running the production
setup steps on the 48 GB Spark installs the DFlash recipe, writes a schema-5 receipt recording
the draft checkpoint at its pinned size and digest, resolves that asset from its own Hugging
Face repository into the shared hub cache, wires
spec-draft-modelto the verified path, andllama-server serves 32 generated tokens from the recipe.
Upgrading an existing pre-bump install is proven on the same device. A receipt rewritten to the
retired b10655 runtime is retained as the upgrade baseline instead of ending setup: the new
runtime installs, the verified model is reused rather than re-downloaded, the receipt is
replaced in place at b11026, and the upgraded install serves generated tokens. This is shown
against a receipt in the exact shape a pre-bump release could have written: schema 4, the
pre-existing default model, and no additional-asset fields. Schema 4 is preserved through the
upgrade, so an install made before this change is not migrated onto the new schema. Full
redacted traces are in the PR comments.
Native x64 is hardware-verified at this head: on an RTX 5090 the production setup steps install
the b11026 CUDA 13.4 x64 runtime, acquire the model, launch the router, and generate tokens. The
pinned binary reports
build 11026, commit b49650adb, matchingLlamaRuntimeCatalog.ReleaseCommitSha.The draft checkpoint's file identity is proven at the native boundary. An unchanged verified
draft loads and serves; a draft whose snapshot entry is replaced after verification is rejected
at reconcile against its pinned size and digest, so llama-server is never launched and the
replaced bytes never reach native I/O. At launch the preset receives the handle-resolved
physical path from that same verification rather than the receipt path.
The b10655 to b11026 upgrade is proven on both architectures: on native x64 (RTX 5090) and on
arm64 (RTX Spark). In each case the pre-bump receipt is retained as the upgrade baseline, the new
runtime installs, the verified model is reused rather than re-downloaded, the receipt is replaced
in place at b11026, and the upgraded install serves generated tokens. Full redacted traces are in
the PR comments.
32 GB RTX Spark behavior
RtxSparkInferenceSelector.SelectDefaultreturns no recommendation below the 32/48 GB geometricmidpoint, surfaced as
LocalInferenceSelectionFailureCode.NotRecommendedForSkurather thanInsufficientGpuMemory, so a product rule is never reported as a capacity failure. In the setup UIthe Local AI option is presented as unavailable and setup proceeds without it. In the pipeline with
Local AI explicitly enabled,
PreflightLocalAiHardwareStepreturns a terminal result; that step isgated by
CanSkip => !ctx.Config.LocalAi.Enabled. A 32 GB Spark is excluded from selection ratherthan failing the host, so a discrete GPU beside it still qualifies through the generic path.
This follows NVIDIA's fixed SKU table, which specifies no local model for the 32 GB SKU. Adopting it
as default setup behavior is a maintainer decision.
Notes for reviewers
The Spark recipes, including DFlash, are reachable from the model picker on
sufficiently large non-Spark GPUs, because
PopulateLocalAiModelsevaluates eachcatalog entry as an explicit request and explicit requests intentionally bypass SKU
routing. The default pick is unaffected and is covered by a regression test. Called
out as known and deliberate rather than silently changed.