[tomastola]

Part of #781. The part that lets rocm serve set the context size is left for a follow-up.

What changes

rocm diagnose --model answered UNDETERMINED for any model outside the curated catalog, and nothing in it took context size into account. For a GGUF model outside the catalog it now gives an estimate instead:

• Where the model comes from. A local .gguf path, or owner/repo:QUANT (or owner/repo:dir/file.gguf) on Hugging Face. For a remote file only the header is read: one repository listing, a HEAD request where the listing does not give a size, then a few MB by HTTP range request. The weights are never downloaded. HF_ENDPOINT selects a mirror; no token is sent, so gated and private repositories cannot be read.

• What is estimated. Weights (exact, from the tensor offsets, with the input embedding left in host memory as llama.cpp does, except for a model with tied embeddings, whose output layer uses that tensor on the GPU; that case is not measured and errs high) + KV cache at the requested context size and KV-cache type + compute buffer + a runtime reserve, compared with the dedicated VRAM of the GPU with the most of it that ROCm can see.

• New flags. --context-size <TOKENS> (default 4096, 1 to 16777216) and --kv-cache-type f16|q8_0|q4_0 (default f16). For a curated model they change nothing, and the report says so.

• What is reported. The result is labelled ESTIMATED (fits / does not fit / cannot estimate), never READY or BLOCKED. It gives the parts of the memory figure, the largest context that fits (never more than the model's trained context), and when the model does not fit, what would make it fit: a smaller context, a q8_0/q4_0 KV cache, a smaller quantization, or partial CPU offload with a layer count. The verdict, the largest context and every suggestion are decided by one predicate on the figures as shown, so they cannot contradict each other. --json carries the same as an estimate object; the fields are documented in skills/rocm-doctor/reference.md.

• What it refuses to estimate, with a reason: a split (multi-shard) file, latent attention (MLA), a header missing a figure the formula needs, an unreadable header. When the host is the problem (no GPU visible, GPU memory unreadable, an APU whose memory pool is not readable yet), the figures are still given with nothing to compare them against.

• No verdict where the figure can be too low. A state-space or hybrid model's recurrent state is not counted, and a mixture-of-experts model's compute buffer was not measured. These get their figures and this GPU's memory, but no fits / does-not-fit verdict (cannot_estimate, reason unmeasured_layout), and the detail says why. A header without the feed-forward or embedding width, which size the compute buffer, is treated the same way (reason missing_metadata, with the figures). A sliding-window model, whose figure can only be too high, keeps its verdict.

• --report stays offline. Under --report the model is not assessed and nothing is fetched, for every model, curated ones included; a note on stderr names the model flags that were ignored. A prepared report never carried the model verdict (it is built from the examination and the matched entry alone), so what --report prints is unchanged. For a curated model the only differences are the skipped assessment and the new stderr note.

• A device mask that hides every GPU now gives "no GPU is visible" for a curated model too, the answer has_usable_amd_gpu already gives. Before, it compared the model with a card of 0 GiB and printed that as a measured figure. The same holds on the one other path where the usable-GPU list comes back empty: a Linux host where amd-smi reported a card but neither the KFD topology nor the DRM cards show an AMD GPU (or a WSL host whose GPU plumbing is not ready). A GPU-required rocm serve launch is already refused there, so "no GPU is visible" now agrees with it. The list is read from sysfs, not rocminfo, so a rocminfo failure does not reach it.

• A note when a device mask is set on a host whose GPUs differ in memory. How HIP_VISIBLE_DEVICES/ROCR_VISIBLE_DEVICES numbering maps onto amd-smi's is not verified on such a host, so the estimate says the card it compared with may be a different one. CUDA_VISIBLE_DEVICES also triggers the note: the HIP runtime honours it, but the memory comparison (like rocm serve's usable-GPU check) reads only the HIP and ROCr variables, so a figure compared under it is just as open to doubt.

  Neither has an e2e scenario, only unit tests: the hosted e2e runners have no GPU, so there is no card for a mask to hide, and the note needs a host whose GPUs differ in memory, which no CI lane has.

• A multimodal projector named by its file name on Hugging Face (owner/repo:mmproj-....gguf) is answered as a projector, with the repository's model files named instead, rather than as a file the repository does not have. A projector is never picked as the model: a label never matches one, and a bare name a model shares with a projector picks the model. The estimate gives it a new cannot_estimate_reason, not_a_model, and the plan asks about the model's own file rather than offering to serve the projector. Scenario diagnose-52 covers it against a stand-in hub that serves the listing. A local projector file is not recognised as one and is read like any other GGUF. rocm serve does not filter projectors, so rocm serve owner/repo:mmproj-....gguf picks that file as the model; that is unchanged here.

• A sized estimate on a host whose GPU memory could not be read keeps the amd-smi metric --json plan a curated model gets there. With no GPU visible it gets the plan a curated model gets there (rocm diagnose to find out why, then ask again). On an APU there is no plan, since no command would change the answer. Where a GGUF plan offers rocm diagnose --model, the command keeps --context-size and --kv-cache-type and quotes the reference for this platform's shell when it needs quoting (PowerShell on Windows, POSIX elsewhere; a plain Windows path is left as it is, so cmd.exe reads it too). Unit-tested only: no CI lane has a GPU whose memory cannot be read.

• Reading a local .gguf keeps its read buffer across the parser's skips. A 300k-token header read from disk took 342 ms before and 38 ms after here (debug build).

• Untrusted input. The header comes from a stranger's file, so the reader bounds header size (64 MiB), layer count, kept metadata and the total time of a remote read (two minutes), refuses a listed file name that could leave the repository or carries a control, bidirectional or zero-width character, and reports a dropped connection or an unexpected 206 as what it is.

How accurate it is

The compute buffer and runtime reserve are calibrated against real loads of two dense full-attention models on one GPU and engine build, with flash attention on: a Llama-3.1-8B-shaped and a Qwen2.5-1.5B-shaped GGUF, each with its own output layer. Across the sixteen loads the estimate was 0.05% to 5.5% above what was used, never below. A file with tied embeddings (no output.weight) was not measured; its embedding is counted on the GPU, which can only overstate. The loads, and how close the estimate came to them, are recorded once, in the gguf_fit.rs module documentation; the README, docs/testing.md, docs/manual-testing.md and the report's standing caveat point there. The check of the largest context on a nearly full card is in docs/manual-testing.md.

Another GPU, engine build or batch size can differ, and so can mixture-of-experts, state-space and sliding-window models. The report says this in a standing caveat and per-model notes. It also says that flash attention is assumed, because without it the compute buffer grows with the context and the figure is too low.

Not in this PR

• Part 4 of diagnose --model: take context size into account, and answer for models outside the catalog #781, a way for rocm serve to set the context size, is not here and is left for a follow-up, so diagnose --model: take context size into account, and answer for models outside the catalog #781 stays open. The report says rocm serve does not take these options yet, and its suggestions name llama.cpp options rather than a rocm serve command.
• APUs answer "cannot estimate" with the figures shown, because the memory pool the engine allocates from (GTT) is not readable yet. diagnose --model: take context size into account, and answer for models outside the catalog #781 names that as a prerequisite.
• vLLM models are not estimated. diagnose --model: take context size into account, and answer for models outside the catalog #781 scopes this to GGUF first.

Testing

• cargo test --workspace --all-targets (including cargo test -p xtask), cargo test -p rocm-core --lib --features e2e-test-hooks, cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --check.

• apps/rocm/tests/diagnose_model_sends_no_token.rs runs rocm diagnose --model owner/repo:QUANT against a stand-in hub with HF_TOKEN, HUGGING_FACE_HUB_TOKEN and ROCM_CLI_HUGGINGFACE_TOKEN set, and checks that the listing, HEAD and range requests all arrive, carry no Authorization or Cookie header and send User-Agent: rocm-cli, and that the answer is the estimate sized from that header (its source, architecture, layer count and exact KV-cache bytes).

• Unit tests for the header reader (malformed and hostile input, every fixed-width value type as a scalar and as an array element), the arithmetic (boundaries, the largest context, every suggestion kind, each refused layout, and the sixteen measured loads, which synthetic headers must reproduce), file selection, the listing and HEAD requests, and the range-request reader against a local stand-in hub.

• Mutation-checked branches. Each of these has a test that fails when the branch is broken on its own, checked by applying the mutation and running the whole rocm-core lib suite (or the rocm bin suite for the two in diagnose_model.rs):

  • percent-encoding of listed file names
  • the deadline on the listing request, and on the HEAD request
  • _ as a label boundary
  • the widest per-layer feed-forward width
  • the note: label on an estimate's warnings
  • the 4096 default context
  • a ceiling equal to the request still printed
  • the trained-context cap on the smaller-cache suggestion
  • either missing width noted
  • the listing's size cap and the range read's length cap (.take)
  • the metadata and tensor count bounds
  • CUDA_VISIBLE_DEVICES in the mask list
  • the order of estimate and mask caveat
  • the context > 0 guard in the largest-context search
  • the running header-size check
  • an array skip that lands one byte short
  • the width of the bool type (the fixed-width table)
  • the letter-before boundary of a quantization label
  • the last -of- in a shard name
  • an Authorization header on the HEAD request (the CLI no-token test)
  • leaving a tied embedding out of the weights

  Other guards have tests but were not mutation-checked one by one.

• e2e scenarios:

  • diagnose-49 and diagnose-50: an estimate from a GGUF header, in JSON and in text.
    • The text label is checked against whether the host compared memory: ESTIMATED where it did, UNDETERMINED where it did not. Where it did, both scenarios require the outcome the reported figures imply (fits or does not fit), worked out from the same rounded parts the verdict uses, so a predicate turned the wrong way fails on a lane that measures its GPU.
    • The GPU-less hosted runners exercise the second branch, and the MI300X self-hosted lane (E2E tests (MI300X, ...)) the first. That lane passed on 3b6b9f3, before the rebase.
  • diagnose-51: --report makes no request to a counting stand-in hub, while the same question without --report does.
  • diagnose-52: a projector named by file name.
  • diagnose-53 to diagnose-56: the context options refused without --model, a curated model told they do not apply, a missing .gguf path, and a bare owner/repo with a context size (no request made).
  • diagnose-57: a quantization that matches nothing lists the repository's files, with the projector marked.
  • The step "no model weights were fetched" was removed from diagnose-49 and diagnose-50. For a local file it could not fail. The remote case's "header only" property is covered by only_the_header_is_requested_from_a_file_far_larger_than_it, a unit test against a stand-in hub.

• A successful remote read (listing, HEAD, range requests) through the CLI is covered by the no-token integration test above, not by an e2e scenario: the stand-in hubs in diagnose-51, diagnose-52 and diagnose-56 answer the listing at most.

• Checked by hand on a real MI300X, as described in docs/manual-testing.md; those loads were of local files. A remote read against huggingface.co itself (Qwen/Qwen2.5-0.5B-Instruct-GGUF:Q4_K_M, which redirects to a CDN) was checked by hand on a host without a GPU: the header was read and sized.

• Locally (Linux, no GPU), one rocm unit test (storage::tests::prune_removes_the_install_it_named_and_not_a_case_twin_of_it) and seven other diagnose- e2e scenarios fail; they fail the same way on main in this environment. Under a threaded cargo test with e2e-test-hooks, examine::tests::live_rocm_repo_paths_in_dir_skips_a_real_bak_file_on_disk failed once and passes on its own; CI runs that suite under nextest, one process per test.

[juhovainio]

I went through this against the saved diff and CI. The approach looks sound (header-only reads, the shown-figure predicate driving the verdict, --report staying offline), but there are three things to sort out before merge.

Needs fixing

1. xtask ascii_output_contract fails on the \u{...} literals in is_safe_listed_name (see inline). This one test is what turns Test (affected crates), windows-build-and-test and Coverage red, and because nextest cancels afterwards, about 167 tests did not run, so the rest of the PR's unit tests are unverified in CI.
2. CodeQL reports "Uncontrolled allocation size" at the Vec::with_capacity in the range reader (inline). It looks like a false positive given the bounds, but clamping makes that visible and clears the check.
3. The description starts with Closes #781, yet it also says part 4 of #781 (a way for rocm serve to set the context size) is not included. Merging would auto-close the issue with that ask undone. Refs #781 / "Part of #781" fits better unless part 4 lands here.

Worth addressing, not blocking

• A device mask that hides every GPU now yields None instead of Dedicated(0.0) for curated models, and the new mask note is user-visible too. Neither is listed under "What changes" in the description, and the first is covered by a unit test only. Please list both and say why there is no scenario.
• The remote Hugging Face flow (listing, HEAD, range) has no end-to-end coverage through the CLI; the hub stand-in in diagnose-50 only answers 404. The description is honest about this; just keep naming the gap.
• Any non-catalog owner/repo:tag now triggers a hub request by default. It is documented in --help and the README, so I read it as deliberate.
• At roughly 8.6k lines (about 5k of them tests) this is hard to review in one go; splitting parser, reader and fit would have helped.

I didn't build or run anything locally, and I haven't re-verified the claim that each guard's test fails when the guard is removed. The MI300X, W7900 and Strix Halo Ubuntu lanes were still pending when I looked.

[tomastola]

Thanks for the careful review. All three blocking points are addressed in 8a3c837 and the description:

1. The ASCII contract failure is fixed (code points instead of \u{..} literals). cargo test -p xtask and the full workspace suite pass locally, apart from one storage test that fails the same way on main here.
2. The range reader's capacity is clamped to the window size before the allocation.
3. The description now says "Part of diagnose --model: take context size into account, and answer for models outside the catalog #781" and leaves the rocm serve context-size part for a follow-up.

The non-blocking points:

• Device mask: both changes are now listed under "What changes", with why there is no scenario (inline reply).
• Remote hub flow: the Testing section now names the gap: nothing goes through the CLI end to end against a hub that answers listings, HEAD and ranges. Those reads are unit-tested against a local stand-in hub.
• Hub request by default for a non-catalog owner/repo:tag: yes, that is deliberate. The question can't be answered without the header. It is one listing, at most one HEAD and a few MB of ranges, with no token sent, and --report makes no request.
• Size: agreed, it's a lot to take in at once. Splitting it now would restart review of code you have already read, and neither the parser nor the reader does anything a user can see without the fit, so I'll keep this one whole and keep follow-ups like the rocm serve part in separate PRs.

[juhovainio]

I re-read this at the current head. Nothing here blocks the merge on correctness, and CI is green, but I'm requesting changes on a few maintainability points that are cheaper to fix now than after the calibration text spreads further. Inline comments below.

• The measured-calibration narrative (the percentages and "fifteen of sixteen loads") is repeated in the gguf_fit.rs module doc, README, docs/testing.md, docs/manual-testing.md and ESTIMATE_CAVEAT. These will drift independently; state it once and link from the others.
• The GGUF header parser keeps two near-identical type-id tables (Reader::value and Reader::array). A new type id has to be added in both.
• The new diagnose glue in apps/rocm/src/main.rs (mask helpers, ModelQuery, the report note) is about 130 lines of non-test code. A separate module fits the repo's guidance better.
• attach_context_estimate is about 110 lines doing three jobs (decide, read, rewrite the readiness per outcome). Splitting decide from apply would make it easier to follow.

One thing I could not confirm from the diff: classify_accelerator_memory now returns None for Some(&[]) from usable_amd_gpu_indices(). If that helper returns an empty list for anything other than an explicit device mask (for example a rocminfo failure), those hosts change from "0 GiB" to "no GPU visible". That is probably the right answer, but please confirm, and confirm CUDA_VISIBLE_DEVICES is honoured there as the mask note claims.

Also noting: the e2e scenarios only reach the cannot_estimate branch on GPU-less runners; the PR text already states this gap, so I'm not asking for more there.

[nowycondro]

🤖 Automated review (Claude Code) · 8a3c837

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change (blind to this PR's existing review discussion). Implements the GGUF header-only context-size/memory estimate from #781 ("diagnose --model: take context size into account, and answer for models outside the catalog"); explicitly leaves part 4 of #781 (giving rocm serve a way to set context size) for a follow-up, matching the PR's own "Part of #781" framing — #781 is still open, so that framing is accurate (no "Closes" mismatch at this head).

Blocking

• select_gguf_file silently excludes any exact filename containing "mmproj", contradicting its own documented contract — crates/rocm-core/src/gguf_source.rs:159-166 (doc: "An exact file name... picks that one," no stated exception), :168-171 (the ggufs candidate list drops every name containing "mmproj" before any match is attempted), :173-187 (both the exact-name branch and its basename-fallback branch search only within that pre-filtered ggufs list).
  A repository whose only file matching a user-typed exact name happens to contain "mmproj" (a standard llama.cpp vision-projector filename) can never be selected that way, and the resulting "no GGUF file matching" error's file list (names_for_error(&ggufs)) is built from the same filtered list, so it won't even show the file exists. No test exercises exact-name selection of an mmproj-named file (the one existing mmproj test, a_quantization_label_picks_one_file_and_never_a_lookalike, only checks label-based matching, where excluding mmproj is correct and intentional).
  Confidence 85 · logic · Fix: either scope the mmproj exclusion to the label-matching branch only (so an exact name always wins, per the doc comment), or state the exception explicitly in the doc comment and in the error's file listing.

• A test added to guard against context stepping below zero doesn't actually kill removal of the guard it documents — crates/rocm-core/src/gguf_fit.rs:2696-2710 (a_trained_context_below_one_step_that_does_not_fit_gives_no_context), guarding crates/rocm-core/src/gguf_fit.rs:428 (while context > 0 && !self.fits(context, kv, available)).
  Hand-traced the test's exact numbers (weights=1000 MiB+0.4, available=2280.5 MiB, trained-context cap 250): the loop reaches context=0 via the is_multiple_of step-rounding branch, and critically self.fits(0, kv, available) independently evaluates true for this data (shown total at zero context is 2280 MiB ≤ 2280.5 MiB available). So removing just the explicit context > 0 && clause and letting the loop fall through to fits(0, …) produces the identical result (None) for this test's inputs — the test does not distinguish "guard present" from "guard absent" here, even though its doc comment ("used to step below zero: a panic in a debug build... a wrap... in a release one") frames it as covering exactly that. It does genuinely kill the separate is_multiple_of step-rounding mutation (reverting that to plain subtraction underflows at 250-256). The actual dangerous case the explicit guard protects — available below even the zero-context total, so fits(0, …) is also false and the loop would underflow — is never constructed by any test in this file.
  Confidence 75 (independently re-derived the same arithmetic by hand against the source; no Rust toolchain was available in this environment to compile and run the mutation directly) · logic/test-quality · Fix: add a case where fits(0, kv, available) is false (i.e. available below the zero-context shown total) so the loop would try to step below zero without the explicit guard, and confirm that test fails when the guard is removed.

Non-blocking

None

Decisions for the author

• Compute-buffer/runtime-reserve calibration rests on one GPU class (MI300X, a 192 GB data-center card) and one engine build — tradeoff. The motivating use cases named in #781 are mostly low-VRAM consumer Radeon cards, where process overhead as a fraction of available memory could behave differently than on a card with far more headroom. The standing caveat and per-model notes are honest about "another GPU... can differ," so this isn't a defect, but worth confirming that shipping with single-GPU-class calibration is the deliberate v1 scope rather than an oversight, and that a consumer-card data point is tracked for a follow-up.
• No e2e scenario for "a device mask hides every GPU" or "device mask on a host whose GPUs differ in memory" — non-blocking-improvement. The PR description explains the gap (no GPU on hosted e2e runners; no CI lane has a host with differing GPU memory), which satisfies this repo's AGENTS.md ("say why in the PR text"). That rationale doesn't currently appear in docs/testing.md or the feature file itself, though — only the PR description, which is harder to find once the PR is closed. A one-line pointer in docs/testing.md's GGUF-estimate section would make the rationale discoverable later.
• This PR is large for one review pass — non-blocking-improvement. ~8,124 insertions across three new modules (gguf.rs 1,410 lines, gguf_source.rs 1,536 lines, gguf_fit.rs 3,185 lines) plus a 932-line change to model_readiness.rs and a 446-line change to main.rs. This review only achieved full coverage by splitting into four independent workers grouped by coupling (header parser + source reader; fit arithmetic; diagnose/CLI integration; docs + e2e). Splitting the PR itself along those same seams (parser, then reader, then fit math) would make future review passes more tractable, as the size alone forced a heavier process here than a typical change needs.
• AcceleratorMemory::Dedicated(_) is matched inside a branch it can never reach — informational, not a defect. crates/rocm-core/src/gguf_fit.rs:741 matches Unknown | Dedicated(_) inside the else of a let Some(available_gib) = memory.measured_gib() else { … }; measured_gib() (crates/rocm-core/src/model_readiness.rs:128) always returns Some for Dedicated(_) (including 0.0), so that sub-pattern can't be hit under current semantics. Harmless (it would do the right thing if ever reached) and arguably reasonable defensive coding against a future change to measured_gib(); flagging only so nobody mistakes it for an independent safeguard against the device-mask regression this PR fixes — that fix lives entirely in model_readiness.rs's classify_accelerator_memory, which the curated-model path and the GGUF-estimate path both consume from one shared value, so they can't disagree by construction.

Positive signals

• crates/rocm-core/src/gguf_fit.rs's single-predicate design (Sizing::fits driving the verdict, max_context, and every suggestion) is backed by a property-style test (the_verdict_the_largest_context_and_the_suggestions_are_one_decision, ~line 2498) sweeping many synthetic shapes and boundary memory values — a genuine, not just claimed, guarantee that these three outputs can't contradict each other.
• The device-mask "no GPU visible" fix (classify_accelerator_memory's fold(0.0, f64::max) → reduce(f64::max) change in model_readiness.rs) correlates GPUs by usage.index rather than position, consistent with this repo's "correlate by id, not position" rule, and both the curated-model and GGUF-estimate code paths read the one HostFacts.accelerator_memory value computed once, so they can't disagree about GPU visibility.
• Untrusted-header handling in gguf.rs/gguf_source.rs is thorough: every stated bound (64 MiB header, layer/entry/array-depth counts, kept-metadata counts, the 2-minute remote-read budget) is enforced at the point it's used rather than just computed, path-traversal and bidirectional/zero-width filename filtering is layered and tested at both ends of its code-point ranges, and dropped-connection/short-body/wrong-range-start/ignored-range all get distinct, specific error text rather than being coerced into one generic error.
• diagnose-50's "--report makes no hub request" e2e scenario (tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:3578-3589) uses a real request counter against a real stand-in TCP listener, and pairs the "stays at zero" assertion with a complementary "the same question without --report does reach the hub" assertion (:3606-3618) — guarding against the first assertion passing vacuously because nothing was ever wired up to begin with.

Deployment notes

• Any non-catalog owner/repo:tag passed to --model now triggers a Hugging Face network request (repo listing, and possibly a HEAD and range requests) by default, where it previously made none. This is documented in --help and the README, but an operator who relied on rocm diagnose being fully offline for unrecognized models should know the default changed; --report is the escape hatch.
• No migration or new required configuration; --context-size/--kv-cache-type are optional with safe, clap-enforced defaults and bounds.

What this covered

Every file in the diff, read completely: README.md, apps/rocm/src/main.rs, crates/rocm-core/src/{gguf,gguf_fit,gguf_source,lib,model_readiness,net}.rs, docs/{architecture,manual-testing,testing}.md, skills/rocm-doctor/reference.md, and the three tests/e2e-cucumber files — plus the surrounding unchanged code needed to judge each (e.g. AcceleratorMemory/measured_gib in model_readiness.rs when reviewing gguf_fit.rs, and the xtask ASCII-output-contract scanner when verifying the bidi/zero-width filter). Reviewed via four independent fan-out workers grouped by coupling (gguf.rs+gguf_source.rs+net.rs; gguf_fit.rs alone, ~3,185 lines, read in full without needing a further split; model_readiness.rs+main.rs+lib.rs+net.rs; docs+e2e), each reporting candidate findings and explicit per-test revert/branch verdicts independently and blind to each other and to this PR's review discussion; none failed or needed respawning. I then independently re-verified the two findings that survive above by re-tracing the relevant source directly (not by trusting the workers' framing), resolving one cross-worker contradiction against the source in the process: a worker scoped to main.rs/model_readiness.rs reported no integration test proves --report skips model assessment, but the diagnose_steps.rs/diagnose.feature worker's scope shows diagnose-50 is exactly that integration test (a real CLI invocation against a real counting stand-in server) — confirmed directly and dropped as refuted. A few other candidates (a docs/architecture.md module-list ordering question, a CodeQL-clamp "already redundant" observation, a missing raw data table in docs/manual-testing.md) were also checked against source and dropped: the architecture.md ordering is explicitly deliberate per the adjacent prose ("the three gguf* modules below"), the CodeQL fix's own commit message already and accurately says the bound pre-existed, and the calibration data the table would have shown is backed by real test assertions in gguf_fit.rs even though no human-readable table duplicates it.

Commits reviewed: 2e514325…8a3c8375 (PR #826 head at review time).

Did not run as dedicated passes: history (git log/git blame) — the three gguf* files are new, and the review budget went to direct source verification of specific candidate findings instead of a separate blame sweep over model_readiness.rs/main.rs/net.rs; prior-changes — this method does not read prior review threads on this or related PRs, so "do earlier review comments apply here" could not be and was not checked. Code-comments was folded into each fan-out worker's per-file pass rather than run separately. CI: all 23 check runs plus the one external status at commit 8a3c8375 are green (including Test (affected crates), windows-build-and-test, Coverage, CodeQL, clippy, and the e2e suite) — no failing or pending checks at the reviewed commit. Ticket #781 was retrieved directly from GitHub (title/body/state); not reconciled against any discussion on PR #826 itself, which was deliberately not read.

---

🤖 Generated with Claude Code

[tomastola]

Pushed bd8015e for the four inline points; I've replied on each. On the question in the review body:

Can usable_amd_gpu_indices() return an empty list without a device mask? Yes, on one other path, but never because of a rocminfo failure: it does not run rocminfo. On Linux it counts GPUs from the KFD topology, falls back to DRM amdgpu cards when KFD shows none, and then applies the mask. It returns Some([]) in two cases:

• HIP_VISIBLE_DEVICES or ROCR_VISIBLE_DEVICES hides every device.
• The count is a definite zero. That means the KFD topology is readable with no GPU node (or there is no /dev/kfd) and DRM shows no AMD card; on WSL, it means the GPU plumbing is not ready.

An unreadable topology with /dev/kfd present gives None, as does a host where neither KFD nor DRM can be read. None keeps every amd-smi row visible, as before. gpu_vram_usage never returns an empty row list (it maps that to None), so the filter can only come up empty through one of those two cases.

In the second case, amd-smi reported a card that neither KFD nor DRM can see. has_usable_amd_gpu() is false there, so a GPU-required rocm serve is already refused, and "no GPU is visible" agrees with it instead of printing 0 GiB as a measurement. I've added this to "What changes".

Is CUDA_VISIBLE_DEVICES honoured? Not by the memory comparison. visibility_mask_from_env reads only ROCR_VISIBLE_DEVICES and HIP_VISIBLE_DEVICES, the same mask rocm serve uses to check for a usable GPU.

The mask note does fire for CUDA_VISIBLE_DEVICES (via diagnose_model::device_visibility_mask_is_set), and that is where it matters most: HIP honours the variable but the comparison ignores it, so the compared card may be one the runtime will not use. The description now says this, and the function's doc comment says why its list is wider than the helper's.

Making the shared helper read CUDA_VISIBLE_DEVICES would also change how rocm serve validates GPUs, so I've kept it out of this PR. I can open an issue for it if you'd like.

bd8015e does not change behaviour. I ran it locally: the workspace tests pass except one storage test that fails the same way on main here, and xtask, clippy with -D warnings and fmt are clean.

[juhovainio]

Looks good to me. This is a diff read; I did not build or run it, and CI was still running on a few lanes when I looked.

It delivers items 1-3 of #781 and is explicit about deferring item 4, APUs and vLLM. Docs, scenarios (diagnose-48 to 50) and file layout follow the repo conventions, and I found nothing blocking. A few non-blocking points:

• The real Hugging Face redirect/range path is only covered by loopback unit tests plus a 404 stand-in in diagnose-50, so nothing in CI reads from the real hub or follows a CDN redirect. Worth a manual check before release.
• --report now ignores --model for curated models too, while the README and PR body describe the change as GGUF-only. Either say so in the docs or confirm it is intended.
• The "mask hides every GPU gives no GPU visible" change for curated models is unit-tested only; the PR body justifies the missing scenario, which is fine, just noting it.
• Design questions for you: the estimate sits under verdict: undetermined, calibration comes from two dense models on one GPU and build, and flash attention is assumed. Those are reasonable for a first cut but worth stating as known limits in the docs.
• Minor: gguf_fit.rs is large and carries about a dozen parallel *_gib/*_bytes Option fields; a small struct could tidy that in a follow-up.

Two small doc nits are inline.

[nowycondro]

🤖 Automated review (Claude Code) · bd8015e

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Review of changes since the review at 8a3c8375 (README.md, apps/rocm/src/diagnose_model.rs [new], apps/rocm/src/main.rs, crates/rocm-core/src/gguf.rs, crates/rocm-core/src/gguf_fit.rs [doc-only diff], crates/rocm-core/src/model_readiness.rs, docs/architecture.md, docs/manual-testing.md, docs/testing.md), plus crates/rocm-core/src/gguf_source.rs and crates/rocm-core/src/gguf_fit.rs read whole and fresh (both carried open findings from the prior round). Implements items 1-3 of #781 ("diagnose --model: take context size into account, and answer for models outside the catalog"); item 4 (giving rocm serve a way to set context size) and APUs/vLLM are explicitly deferred, matching #781's own scope note — #781 stays open, and the PR description's "Part of #781" framing is accurate.

Blocking

• select_gguf_file silently excludes any exact filename containing "mmproj", contradicting its own documented contract — crates/rocm-core/src/gguf_source.rs:167-213 (doc comment: "Pick the GGUF file a variant names... An exact file name, or a quantization label," no stated exception). The candidate list (ggufs, line ~171) drops every name containing "mmproj" before either the exact-name branch or the label-matching branch runs, and the "no GGUF file matching" error's file listing (names_for_error(&ggufs)) is built from that same filtered list. A repository whose only file matching a user-typed exact name happens to contain "mmproj" (a standard llama.cpp vision-projector filename) can never be selected that way, and the error won't even show the file exists. The one existing mmproj test (a_quantization_label_picks_one_file_and_never_a_lookalike) only exercises label-based matching, where excluding mmproj is correct; no test exercises exact-name selection of an mmproj-named file. This file was unchanged since the prior round; re-derived fresh by reading the whole file blind.
  Confidence 85 · logic · Fix: scope the mmproj exclusion to the label-matching branch only (so an exact name always wins, per the doc comment), or state the exception explicitly in the doc comment and the error's file listing.

• The test guarding "context stepping below zero" doesn't actually kill removal of that guard — crates/rocm-core/src/gguf_fit.rs:2709-2723 (a_trained_context_below_one_step_that_does_not_fit_gives_no_context), guarding the while context > 0 && !self.fits(context, kv, available) loop in Sizing::max_context (around line 436). Hand-traced the test's own numbers (weights = 1000 MiB + 0.4 bytes, available = 2280.5 MiB, cap Some(250)): the loop steps context from 250 to 0 via the is_multiple_of rounding branch, and fits(0, kv, available) independently evaluates true here (shown_total(0,…) = 2280 MiB <= 2280.5 MiB), because the test's +0.4 byte remainder on weights rounds away rather than pushing mib_up to the next whole MiB. So removing just the context > 0 && clause produces the identical result for this test (the loop still stops at context == 0 via the !fits(0,…) becoming false on its own) — the test cannot distinguish guard-present from guard-absent. The genuinely dangerous case (available below even the zero-context shown total, so fits(0,…) is also false and the loop would underflow) requires shown_total(0,kv) to exceed fixed() via mib_up's rounding up — achievable by giving weights (or compute_base) a remainder just over a MiB boundary rather than the test's +0.4 (which rounds down) — and no test in this file constructs it. This file also changed in the latest commit, but only by added doc comments (verified they don't contradict the code or the sixteen calibration loads they describe); this test and the loop it guards are unchanged since the prior round.
  This directly engages AGENTS.md §3 ("A new test is shown to fail when the code it covers is broken... A test that stays green with its target removed is not coverage").
  Confidence 80 (hand-traced the exact arithmetic against the source; no Rust toolchain available in this environment to compile and run the mutation) · logic/test-quality · Fix: construct a case where fits(0, kv, available) is false (give a fixed component a byte remainder that rounds up via mib_up, with available between the raw fixed() sum and the rounded shown_total(0,kv)), and confirm that test fails when the explicit guard is removed.

Non-blocking

None

Decisions for the author

• Compute-buffer/runtime-reserve calibration rests on one GPU class (MI300X, a 192 GB data-center card) and one engine build — tradeoff. #781's motivating cases are mostly low-VRAM consumer Radeon cards (R9700, 7900 XTX, 5700, 9060 XT, an 890M APU), where process overhead as a fraction of available memory could behave differently than on a card with far more headroom. The module doc, README and ESTIMATE_CAVEAT are all honest that "another GPU... can differ," so this isn't a defect — worth confirming this is the deliberate v1 scope rather than an oversight, and that a consumer-card data point is tracked for a follow-up.

Positive signals

• The latest commit's maintainability refactor is behavior-preserving, confirmed by independent re-reading rather than trusting its "no change in behaviour" claim: attach_context_estimate's split into plan_context_estimate/read_context_estimate/apply_context_estimate (model_readiness.rs) traces to an identical set of branches and readiness mutations as before for every combination of catalog/undetermined-reason/reference-shape state; the apps/rocm/src/main.rs → diagnose_model.rs extraction moved KvCacheTypeArg/ModelQuery/report_ignores_model_note/attach_gguf_estimate with every call site updated and no leftover dead code or unused imports; and gguf.rs's merge of the two near-identical type-id match tables into one fixed_width() function is exercised correctly for every type id by signed_values_of_every_width_decode_by_their_own_sign_bit, which checks both ends of every width (1/2/4/8 bytes, signed and unsigned).
• gguf_fit.rs's single-predicate design (Sizing::fits driving the verdict, max_context, and every suggestion) is backed by a genuine property-style sweep (the_verdict_the_largest_context_and_the_suggestions_are_one_decision) over many synthetic shapes and boundary memory values, not just a claim.
• Untrusted-header/network handling in gguf_source.rs is thorough and was independently re-verified: is_safe_listed_name's bidi/zero-width filter is tested at both ends of every excluded code-point range, the 2-minute total-read budget is enforced ahead of each request (not just summed afterward), and a wrong range-start, a short body, and a short-but-consistent body each get distinct, specific error text rather than one generic "truncated."

Deployment notes

• Any non-catalog owner/repo:tag passed to --model triggers a Hugging Face network request by default (listing, possibly HEAD and range requests), documented in --help and the README.
• No migration or new required configuration.

What this covered

DEGRADED REVIEW (partial fan-out). I attempted a 5-way fan-out (gguf_source.rs; gguf_fit.rs; model_readiness.rs+diagnose_model.rs; main.rs; gguf.rs+docs), but this environment had ~20+ unrelated subagents already running under the same session-level concurrency ceiling before I started (not spawned by this review), so 4 of 5 spawn attempts were rejected outright and the 1 that did launch (gguf.rs + docs) had not returned after 8+ minutes by the time I needed to publish. I read and reviewed all five groups directly in this single top-level context instead — including gguf.rs and the four doc-diff files the unreturned worker was also assigned — so in substance this ran as a single-context review, not an independently cross-checked one; treat it accordingly.

Read completely and fresh: crates/rocm-core/src/gguf_source.rs (1536 lines) and crates/rocm-core/src/gguf_fit.rs (3198 lines) in full, blind to the files' own history; the diff since 8a3c8375 in README.md, apps/rocm/src/diagnose_model.rs, apps/rocm/src/main.rs (plus enough of the surrounding 9723-line file to confirm every call site of the moved items was updated and no import went unused), crates/rocm-core/src/gguf.rs (full file, 1412 lines), crates/rocm-core/src/model_readiness.rs (full file, 2225 lines), and the four changed doc files. Commits reviewed: 8a3c8375…bd8015e3 for the diff, full-file reads at bd8015e3.

One integrity check worth recording: midway through, a locally-cached copy of main.rs was found to contain a different commit's content (likely overwritten by one of the unrelated concurrent agents sharing this environment's tool-result cache) before I had drawn any conclusion from it; it was re-fetched and re-verified against the bd8015e3 SHA before use.

Contamination disclosure. The mandated claim-check (confirming no existing review already covers this exact head) required fetching this PR's review list, which necessarily surfaced the full prior automated review's text, including the verbatim wording of the two findings above, before I re-read gguf_source.rs and gguf_fit.rs myself. I re-derived both independently — tracing select_gguf_file's control flow and hand-computing the max_context test's arithmetic from scratch against the current source, rather than asserting the prior wording — but a fully blind re-derivation this was not, and that limits how much weight "re-found independently" should carry here.

Did not run as dedicated passes: history (git log/git blame) and prior-changes (do earlier review comments apply here) — this method does not read prior review threads, so the latter could not be checked at all. Code-comments was folded into the per-file reads rather than run separately. CI: all 23 check runs (including Test (affected crates), windows-build-and-test, Coverage, CodeQL, clippy, and the e2e suite) are green at bd8015e3, re-confirmed immediately before publishing. Issue #781 was retrieved directly (title/body/state); not reconciled against any discussion on PR #826 itself beyond the mandated claim-check above.

---

🤖 Generated with Claude Code

[jussielo-amd]

Requesting changes. The approach is sound and several pieces are well tested (one predicate decides the verdict, the largest context and every suggestion, backed by a sweep test; diagnose-50 pairs "no request under --report" with "the same question without --report does reach the hub"). The blockers are about the branches this PR adds that no test pins, tests that cannot fail on what they claim, scenario coverage for observable behaviour, and one claim in the description.

Reviewed at bd8015e. Line numbers below are at that head. I broke each branch in a scratch copy and ran the lib tests; "survives" below means the whole rocm-core suite (1067 tests) stayed green with the branch broken, including when all the rocm-core mutations were applied at once.

Blocking

1. The description says each guard's test was shown to fail when the guard was removed, and that does not hold. In "Testing". Removing the listing-request timeout (gguf_source.rs:338), the HEAD-request timeout (gguf_source.rs:531) or the percent-encoding of listed file names (gguf_source.rs:493) all survive. The "Untrusted input" bullet presents the two-minute read bound and the file-name handling as guarded. Either add the tests in 2, or narrow the claim.

2. Branches this change adds are pinned by no test. Each of these survives a single-branch mutation:

• gguf_source.rs:493: encode_segment replaced by to_owned. No fixture name needs encoding, so add a fake-hub name with a space or + and assert the request line.
• gguf_source.rs:338, :531: timeouts removed. the_whole_read_has_a_deadline_not_only_each_request only drives RangedReader.
• gguf_source.rs:153: is_boundary(before, &['_']) changed to &[]. Every fixture puts - before the label, so model_Q4_K_M.gguf is untested.
• gguf.rs:418: .max() changed to .min() for an array-valued feed_forward_length. The compute buffer is sized from it.
• diagnose_model.rs, device_visibility_mask_is_set and attach_gguf_estimate: deleting CUDA_VISIBLE_DEVICES from the list survives, and so does swapping the two calls in attach_gguf_estimate (with the mask note first the estimate is still None, so the caveat never fires). The README and --help promise this note, and the_unverified_mask_note_reaches_an_estimate_when_the_gpus_differ passes mask_set straight to note_mask_for_rows, so it never sees the env reader or the call order. An injectable lookup, plus one test driving attach_gguf_estimate with a fake GgufSource and two unequal-size rows, would cover both.
• model_readiness.rs:1037-1041: label forced to recipe note. contains("note: these are estimates") is also a substring of recipe note: these are estimates, so assert the exact prefix and !contains("recipe note").
• model_readiness.rs:702: DEFAULT_CONTEXT_SIZE changed from 4096 to 2048. The test at :1820 compares the constant with itself, while --help and the README state 4096 independently.
• gguf_fit.rs:1011: < changed to <=, which hides "largest context that fits" when the request equals the ceiling exactly.
• gguf_fit.rs:823: the trained-context cap dropped from the smaller-cache suggestion. The test uses 0.6 GiB of room, so the cap never binds.
• gguf_fit.rs:721: || changed to && in the missing-width note. No test has exactly one width missing.

The new tests call new API, so reverting the whole change fails to compile. The single-branch mutations above are the check that matters.

3. Three tests cannot fail on what they claim.

• The step no model weights were fetched (diagnose_steps.rs:2082-2105, used by diagnose.feature:749 and :759) asserts two directories do not exist. A local-file estimate only calls File::open, so it cannot go red. No scenario drives a successful remote read, as the description admits, so the claim is unasserted where it matters.
• gguf.rs:1106, a_header_that_spans_more_than_the_size_bound_is_refused, asserts contains("is larger than"), which both the skip and check_size messages contain. The fixture reaches skip first, so the check_size guard in the name is never exercised.
• gguf.rs:892, arrays_are_skipped_not_stored...: the name says the array length is what is kept, but it never reads the llama.after_the_arrays value it builds.

Please assert specific behaviour, or rename.

4. Observable behaviour that only has unit coverage, with no argument in the description (AGENTS.md section 3). These are deterministic and host-independent, like diagnose-48 and diagnose-49:

• --context-size or --kv-cache-type without --model (exit 2).
• The curated-model line saying the context options do not apply.
• A missing local .gguf path, which now says "could not be opened" and no longer offers the upstream-tracker route.
• Bare owner/repo with --context-size.

The description argues the mask items and the remote read, and I accept those. Separately, diagnose-49's ESTIMATED label is only reached on lanes that measure GPU memory: I made header_label never return ESTIMATED and both new scenarios stayed green on a GPU-less host. Please name the lane that covers it.

Non-blocking

• Local .gguf header parse is about 20 times slower than it needs to be. gguf_source.rs:468-472 wraps the file in a BufReader, and the string-array loop at gguf.rs:348-355 skips each token with a seek, which drops the buffer every time. I measured a 300k-token header at 5.3 s in a debug build against 0.24 s through a Cursor. Real vocabularies are 128k to 262k tokens, so diagnose --model ./x.gguf stalls for seconds. seek_relative, a larger buffer, or reading the header span into memory would fix it.
• A ROCR_VISIBLE_DEVICES mask can make the estimate compare the wrong card. usable_amd_gpu_indices_from re-indexes ROCR survivors to 0..N (host_gpu.rs:1397-1424), while classify_accelerator_memory matches against amd-smi's usage.index (main.rs:3193-3197). I read that as the physical ordinal but did not check it against amd-smi. If so, ROCR_VISIBLE_DEVICES=1 on a 24 GiB plus 8 GiB host picks the 24 GiB row, and the header can read ESTIMATED: fits with the caveat only at the bottom. Consider CannotEstimate on a mixed host with ROCR set, or translating the ordinals.
• The device-mask variable list is stated in four places (device_visibility_mask_is_set, MASK_UNVERIFIED_NOTE, the --model help and the README). They agree today; section 3 asks for one shared constant and a test tying producer to consumer.
• LiveGgufSource::with_hub (gguf_source.rs:315) and RangedReader::fetched (:576) are pub but only tests call them. Several tests also build a manual temp dir that a failed assertion leaves behind, though tempfile is already a dependency.

Decisions for you

• Calibration rests on one GPU class (MI300X) and one llama.cpp build, while #781's audience is mostly consumer Radeon cards. The caveat is honest; please confirm this is the intended v1 scope.
• The suggestions name llama.cpp options, including --n-gpu-layers, for a knob rocm serve does not expose yet. That is advisory text, not a silent CPU fallback, but sections 6 and 7 are strict about the no-CPU-fallback policy, so please confirm it is intended.
• shard_parts and first_shard_of_one_set pick a first shard, then gguf_fit.rs:469 refuses every split.count > 1. The machinery only changes which message the user sees.
• The new clamp in 8a3c8375 has an else arm that cannot run, since wanted is already at most WINDOW_BYTES. The comment explains it is there for the analyser; I am noting it as a deliberate tradeoff.
• A remote read can sit silent for up to two minutes, and diagnose was offline before. Section 6 points to AnimatedSpinner for progress that goes quiet.

Not covered

I did not run the GPU or Windows lanes, the real Hugging Face path or workspace-wide clippy and coverage, and I relied on CI for those. The calibration-figures duplication I would otherwise have raised is fixed in bd8015e. I did not finish a line-by-line review of the new fixed_width table in gguf.rs or the plan/read/apply split of attach_context_estimate in model_readiness.rs; the existing suite stays green over them, and that is all I checked there. The moved main.rs glue keeps its behaviour and its tests, and none were dropped.

[tomastola]

@nowycondro, replying to your automated review at 8a3c837:

Thanks. Both blocking findings held against the code, and both are fixed in 3cff3aa.

mmproj and exact names. You're right: the projector filter ran before the exact-name match, so naming a projector said "no GGUF file matching" and left it out of the list. Picking it as the model would be wrong too, since a projector isn't a language model. Now a name that matches only projectors gets its own error ("names a multimodal projector (mmproj), not a language model"), which lists the repository's model files. A projector is still never picked: a label never matches one, and a bare name that a model and a projector share picks the model, as before. The match ignores case, and the doc comment now states the rule.

The refusal is a typed error, so the estimate reports it with its own cannot_estimate_reason, not_a_model, rather than as a header that could not be read. That is a new value in the JSON enum, listed in skills/rocm-doctor/reference.md. The plan used to fall back to "serve it and read the engine's refusal", which would have named the refused projector in a rocm serve command. Now it asks for the model instead: by its exact file name when the repository holds one model whose name reads back as that hub file (verify after fix: rocm diagnose --model owner/repo:<file>.gguf), and with no command otherwise. The detail line lists the models. A projector-like name that failed for another reason, such as an unreachable hub, keeps the ordinary unreadable answer. The README and the manual QA list now mention the refusal. Its scope is a projector named by its file name on Hugging Face. A local projector file is still read like any other GGUF, and a quantization label that matches only a projector still reads as matching nothing. Both are documented as such, and could be a follow-up (for example, checking general.architecture once the header is read).

Unit tests cover each of those cases, and I confirmed they fail when the case-insensitive match, the model-only list, the shared-name rule, the typed refusal or the concrete verify line is removed. The new scenario diagnose-51 serves a listing with a model and its projector from a stand-in hub, and asserts the message, that the plan names the model file and not the projector, and that only the listing was requested. It fails against the old selection code.

One difference this leaves: rocm serve's own file selection doesn't filter projectors, so rocm serve owner/repo:mmproj-x.gguf picks that file as the model. I've noted that in the description rather than change serve here.

The below-zero guard. Confirmed by mutation: with context > 0 && removed, all 58 gguf_fit tests still passed. The new test when_even_zero_context_does_not_fit_the_search_stops_at_zero puts the card's memory between the exact and the shown zero-context totals, so fits(0) is false and the early return does not fire. Without the guard it panics with "attempt to subtract with overflow". The older test's comment now says it covers the step-rounding branch, not the guard.

On the rest:

• One GPU class for calibration. That is the deliberate scope for this first version, and the standing caveat says so in every report. A consumer Radeon data point is the obvious next measurement; docs/manual-testing.md §10 asks whoever runs it to report the model and both figures. It isn't tracked in an issue yet.
• No e2e scenario for the mask cases. The reason is now also in docs/testing.md, next to the other GGUF-estimate tests, not only in the PR description. That bullet and the new max_context test answer this review, which is why they are in this commit beside the projector change.
• PR size. Fair. I'm not splitting it at this stage of review, but parser, reader and fit are separate modules with separate tests, if that helps a reader.
• Dedicated(_) in the unreachable arm. Agreed, it is defensive and not a safeguard of its own; the mask fix lives in classify_accelerator_memory, as you say. I've left it.
• Network by default for owner/repo:tag. Deliberate, and documented in --help and the README. --report stays offline.

[juhovainio]

Read through the whole diff at this head and found nothing blocking. CI is green across all 24 checks.

What I liked: the GGUF parser bounds every hostile-input axis and never reads tensor data; a single predicate drives the verdict, the largest context and the suggestions, so the output can't contradict itself; and scenario diagnose-50 proves --report stays offline with a request-counting stand-in hub.

Non-blocking notes, none of which need to hold the merge:

• The estimator allowances (runtime reserve, compute buffer) are calibrated on one MI300X with one llama.cpp build, while #781's audience is mostly Radeon cards. The caveat is shown and documented, but a manual check on a Radeon would be a nice follow-up.
• The suggestion text names llama-server --ctx-size; in recent llama.cpp builds that value is shared across --parallel slots. Right for a single slot, but the report doesn't say so. I haven't verified this against the pinned build.
• list_files trusts the ?blobs=true siblings list to be complete; for very large repos that may not hold. Also unverified.
• The remote read follows redirects and re-sends the Range header to the CDN. No credentials are sent, just confirm that's intended.
• The device-mask fix for curated models is a separable change that could have been its own PR, though it's called out in the body and unit-tested.

One question inline.

[tomastola]

@nowycondro, a follow-up on both findings, now in f84afbf.

mmproj: the errors name the real file.

• Refusal. It names the file the reference resolved to. MMPROJ-Q8_0.gguf gives "vision/MMPROJ-Q8_0.gguf is a multimodal projector (mmproj), not a language model, so MMPROJ-Q8_0.gguf cannot be estimated", followed by the repository's model files.
• No-match. It lists every GGUF in the repository, projectors included and marked "(multimodal projector)". Before, it listed only the models, so a projector that exists was missing from the list. That covers a label such as F16 that matches nothing but the projector.

Why an exact projector name is refused rather than selected. A projector is the vision encoder that ships beside a llama.cpp model. It is not a language model, and this command estimates a language model's weights plus KV cache at a context size.

• The header doesn't fit the formula. llama.cpp writes projectors with architecture clip and clip.vision.* keys, not the <arch>.block_count, attention-head and context-length keys the estimate reads. That is from llama.cpp's key layout; I have not parsed a real projector file here.
• Selecting it gives no figure. It would end as cannot_estimate / missing_metadata after a header fetch. The plan would then be "serve this model directly", which hands the projector to rocm serve as if it were the model.
• Refusing is cheaper and points somewhere useful. It costs only the listing request (diagnose-51 asserts exactly one request) and names the model to ask about instead. It is reported as cannot_estimate_reason: not_a_model, not as an unreadable header.

The zero-context guard. Done as you described. when_even_zero_context_does_not_fit_the_search_stops_at_zero gives the weights half a MiB over 1000 MiB, so mib_up rounds that part up to 1001. The exact fixed sum is then 2280.5 MiB and the shown zero-context total is 2281 MiB. available is 2280.75 MiB, between the two, so the early return does not fire and fits(0, kv, available) is false. The test asserts both preconditions.

Mutation evidence, rerun at the current head: I removed context > 0 && from while context > 0 && !self.fits(context, kv, available) and ran cargo test -p rocm-core --lib gguf_fit:

test gguf_fit::tests::when_even_zero_context_does_not_fit_the_search_stops_at_zero ... FAILED
panicked at crates/rocm-core/src/gguf_fit.rs:455:17   (attempt to subtract with overflow: `context - CONTEXT_STEP` at 0)
test result: FAILED. 58 passed; 1 failed

Only this test fails, so it is the one that pins the guard. The older a_trained_context_below_one_step_that_does_not_fit_gives_no_context stays green under that mutation, as you found. Its comment now says it covers the step-rounding branch. With the guard restored, all 59 pass.

[tomastola]

@jussielo-amd, thank you, this was a thorough review. Everything below is in f84afbf. I confirmed each finding by mutation before fixing it, and the description is corrected.

How I checked. A script applies one mutation at a time to the working tree, runs the whole rocm-core lib suite (or the whole rocm bin suite for the two in diagnose_model.rs), records the result and restores the file.

1 and 2: branches no test pinned. All twelve you listed survived at 130b5c1, as you said. Each now has a test that fails when the mutation is applied alone:

Mutation	Now killed by
encode_segment → to_owned	a_listed_file_name_is_requested_percent_encoded (the request line must be dir/m%2Bx-Q4_K_M.gguf)
listing timeout removed	the_listing_request_is_bound_by_the_deadline (a spent deadline makes no connection; a stalled hub is cut off in under 2 s)
HEAD timeout removed	the_head_request_is_bound_by_the_deadline (same, for head_size)
is_boundary(before, &['_']) → &[]	a_label_after_an_underscore_is_matched (model_Q4_K_M.gguf)
.max() → .min() for an array feed-forward width	a_per_layer_feed_forward_width_is_read_as_the_widest
warning label forced to recipe note	the estimate test now asserts a line starting note: these are estimates and no recipe note
DEFAULT_CONTEXT_SIZE 4096 → 2048	the plan test and the_default_context_is_labelled_as_a_default now assert the literal 4096 that --help and the README state
< → <= at the ceiling	a_request_at_exactly_the_ceiling_still_prints_the_ceiling
trained-context cap dropped from the smaller-cache suggestion	a_smaller_cache_suggestion_caps_its_largest_context_at_the_trained_one (the uncapped q4_0 ceiling is asserted to exceed 8192, so the cap binds)
|| → && in the missing-width note	a_header_missing_either_width_says_the_compute_buffer_is_understated (each width missing alone)
CUDA_VISIBLE_DEVICES deleted from the list	each_visibility_variable_alone_is_a_mask_and_the_caveat_names_it
the two calls in attach_gguf_estimate swapped	attaching_an_estimate_under_a_mask_on_mixed_gpus_carries_the_caveat

For the last two, the environment lookup is now injectable (mask_set_where). The new test drives attach_gguf_estimate_with with a fake GgufSource serving a real header and two cards of different sizes, so it sees both the env list and the call order. The three variable names are now one constant, and the test also checks that each appears in MASK_UNVERIFIED_NOTE. That ties the producer to the consumer, which was your non-blocking point about four copies of the list. The --model help no longer names them (since 130b5c1), and the README is prose about them.

On the description. It was wrong, and it's corrected. The Testing section now says plainly that the earlier "each guard's test was shown to fail" was not true, lists the mutations that were checked and how, and says the other guards were not checked one by one.

3: tests that could not fail.

• "no model weights were fetched". Removed from diagnose-48 and diagnose-49: for a local file it could not go red. The remote "header only" property stays covered by only_the_header_is_requested_from_a_file_far_larger_than_it, against a stand-in hub that records every range and claims an 8 GiB file. The step stays on diagnose-21, a curated model, where it is the pre-existing check of a different path.
• a_header_that_spans_more_than_the_size_bound_is_refused. It now uses two arrays, each under the bound, that together pass it, and asserts the running check's own message ("the header is larger than"). Removing the check_size after a metadata key now fails it. The single-length case stays with a_single_skip_longer_than_any_header_is_malformed.
• arrays_are_skipped.... Renamed parsing_resumes_exactly_after_arrays_it_skips. It places a kept key (llama.attention.key_length = 99) after the two skipped arrays and asserts it reads back as 99. An array skip one byte short now fails it. The old key could never be read back, because the parser does not keep it.

4: scenarios for observable behaviour. I added four host-independent scenarios. Breaking the behaviour each covers fails it (all four at once, in one build):

• diagnose-52. --context-size or --kv-cache-type without --model exits 2 and names --model. Fails without requires = "model".
• diagnose-53. A curated model asked about --context-size says the options don't apply, and carries no estimate.
• diagnose-54. A .gguf path that does not exist says "could not open ". There is no upstream route, and the plan's verify is rocm diagnose --model <path>, not rocm serve. Fails with readiness.route = None removed.
• diagnose-55. A bare owner/repo with --context-size is told to add :QUANT, and a counting stand-in hub records no request.
• diagnose-56. A quantization that matches nothing lists the repository's GGUF files, with a projector marked as one. That listing changed in this round.

The ESTIMATED label in diagnose-49. You're right that the step accepted either label on any host. It now decides by whether the report compared memory: a compared estimate names the card it was compared with. Compared means the header must read ESTIMATED; not compared means UNDETERMINED.

• The GPU-less hosted runners take the second branch.
• The MI300X self-hosted lane (E2E tests (MI300X, ...) in e2e-selfhosted.yml) takes the first; Strix Halo is an APU and answers undetermined by design.
• That lane has not completed a run on this PR yet (each was cancelled by a newer push), so I can name it but not show it green.
• I could not exercise the compared branch in this environment: there is no GPU, and the low-VRAM test hook's single card is filtered out by the empty usable-device list.

Non-blocking.

• Slow local parse. Confirmed and fixed. The local reader is now a small RelativeSeekBuf: relative seeks use seek_relative, which keeps the buffer, and it tracks its own position, because BufReader::stream_position also asks the file and the parser asks after every skip. A 300k-token header read through the real local open path took 342 ms before and 38 ms after (debug build, this container). I didn't reproduce your 5.3 s, so I'd put the gain at about 9× here, not 20×. a_local_header_is_skipped_through_without_seeking_the_file_per_token counts seeks reaching the file (under 500 for 50 000 tokens) and fails when seek_relative is replaced with seek.
• ROCR_VISIBLE_DEVICES and the wrong card. I agree with the analysis, but not with fixing it in this PR. The comparison you point to (usable_indices.contains(&usage.index) in classify_accelerator_memory) is unchanged from main; it came with feat(diagnose): answer whether a model will run before downloading it #407 and is the same one a curated model's verdict uses. Mapping ROCr survivors back onto amd-smi's ordinals needs a check on a multi-GPU host I don't have. Turning it into CannotEstimate here would leave the curated path disagreeing with this one. This PR's answer is the caveat: on a host whose cards differ, any of the three variables puts MASK_UNVERIFIED_NOTE on the estimate. Fixing the mapping for both paths would be a separate change; I can open an issue for it.
• with_hub and fetched. Both are now #[cfg(test)]. I left the hand-made temp dirs in the existing tests as they are rather than churn them in this round.

Decisions for me.

• Calibration scope. Yes, one GPU class and one llama.cpp build is the intended first version. The caveat says so, and a consumer Radeon data point is the next measurement.
• --n-gpu-layers in the suggestions. Intended. It is advisory text naming llama.cpp's own flag, marked as not a rocm serve option, and nothing falls back to the CPU.
• Shards. Right: picking the first shard only changes which message you get. A split model gets "split file, cannot estimate" rather than "ambiguous", which I think is the more useful answer; the machinery is small.
• Clamp else arm. Deliberate, for the analyser, as you read it.
• No spinner during a remote read of up to two minutes. Agreed it would help, but I've left it out of this round. It changes what the command prints while it waits, and I'd rather add it with AnimatedSpinner in a follow-up than grow this PR again.

[tomastola]

One more push after the replies above: bd91383 answers @juhovainio's inline question on the plan for an estimate that cannot compare (details in that thread). Nothing else changed.

[juhovainio]

Re-reviewed at the current head. No blocking findings; steps 1-3 of #781 are in and the PR correctly leaves the issue open for the rocm serve context option.

Non-blocking:

• --report now skips --model for curated models too, though only the GGUF path can touch the network (inline). Worth confirming report.model output for a curated --report --model is unchanged, or calling the change out.
• SSM/hybrid models still headline ESTIMATED: fits with the recurrent state uncounted (inline). MoE has the same shape: the figure can be low and only a note line carries the caveat.
• The PR body still carries review-round history ("an earlier version said... not true"). Per the repo's guidance it should read from the final diff.
• The curated-model empty-mask classify_accelerator_memory change and the RelativeSeekBuf read optimisation are separable from the estimator and could be their own PRs.

Questions: calibration is two dense models on one GPU and engine build, and the estimate compares against the single largest GPU only. Both are fine for a first cut, but worth stating in the docs as the estimate's limits.

CI had no failures when I looked; the MI300X lane had not yet run, so the compared branch of diagnose-48/49 is unexercised so far.

[tomastola]

@juhovainio, thanks for the re-review. On the points in your review body (the inline ones are answered in their threads):

• Review-round history in the description. Removed. The Testing section now describes the final state: what was mutation-checked, and what was not.
• Limits in the docs. Both are stated in the README's estimate section. The calibration is from two dense models on one GPU and llama.cpp build, with the figures in the gguf_fit module documentation. The comparison is with "the total dedicated VRAM of the GPU with the most of it that ROCm can see". I'll add anything else you'd like spelled out.
• Separable changes. Fair. The empty-mask classify_accelerator_memory change and RelativeSeekBuf could each stand alone. I've kept them here because a reviewer asked for both in this PR, and splitting now would mean re-reviewing them on their own. If you'd rather have them out, I'll move them.
• MI300X lane. Agreed: the compared branch of diagnose-48/49 stays unexercised until that lane runs on this head.

[nowycondro]

🤖 Automated review (Claude Code) · cf654f9

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — no blocking findings

Review of changes since the review at bd8015e3 (README.md, apps/rocm/src/diagnose_model.rs, apps/rocm/src/model_readiness.rs, crates/rocm-core/src/gguf.rs, crates/rocm-core/src/gguf_fit.rs, crates/rocm-core/src/gguf_source.rs, docs/manual-testing.md, docs/testing.md, skills/rocm-doctor/reference.md, the three tests/e2e-cucumber files), plus a small diff-with-context read of apps/rocm/src/main.rs. gguf_source.rs and gguf_fit.rs both carried open findings from the prior round and both changed again in this window, so both were read whole and fresh rather than trusted from before. Implements items 1-3 of #781 ("diagnose --model: take context size into account, and answer for models outside the catalog"); item 4 (giving rocm serve a way to set context size), APUs and vLLM are explicitly out of scope and #781 stays open — the PR's "Part of #781" framing is accurate, no "Closes" mismatch.

Blocking

None. Both findings open from the prior round are resolved at this head, independently re-verified against the source rather than taken on trust:

• select_gguf_file (crates/rocm-core/src/gguf_source.rs:224-275) now searches the exact-filename branch over every GGUF file including projectors (all_ggufs), filters to non-projector models only afterward, and returns ProjectorNamed only when the exact name matched exclusively projector(s) — a model sharing a name with a projector is now selected correctly, and the "no match" error listing also now shows projector files instead of hiding them. Confirmed by direct re-read, not by trusting the commit message.
• Sizing::max_context's zero-context guard (crates/rocm-core/src/gguf_fit.rs, the while context > 0 && !self.fits(...) loop) is now backed by a genuinely discriminating test, when_even_zero_context_does_not_fit_the_search_stops_at_zero, hand-traced end to end: with the guard removed, context would reach 0u64 - 256, an underflow. The previously-weak test (a_trained_context_below_one_step_that_does_not_fit_gives_no_context) was also updated to exercise the "round onto a whole step directly via integer division" fix rather than a step that happened to not distinguish guard-present from guard-absent.

Non-blocking

• UnmeasuredLayout's GiB→MiB conversion omits this file's own established rounding epsilon. crates/rocm-core/src/gguf_fit.rs:795-799 computes (available_gib * 1024.0).floor() for the "compare it with this GPU's X MiB yourself" detail string on the SSM/MoE no-verdict path. Every other GiB→MiB floor conversion in this file (e.g. crates/rocm-core/src/gguf_fit.rs:1040, let offered = available.mul_add(1024.0, 1e-6).floor();) uses a 1e-6 epsilon specifically to stop floating-point representation error from understating the host's memory by 1 MiB — explained by its own adjacent comment ("a host never reads as offering more than it does"). The new line is display-only (not read by any verdict/arithmetic decision) and would only show 1 MiB less than the true figure in a rare float-representation edge case, but it's an unexplained inconsistency against a pattern this same file established for the identical computation.
  Confidence 85 · mechanical · Fix: use available_gib.mul_add(1024.0, 1e-6).floor() here too, or factor the GiB→MiB-floor step into one shared helper used by both call sites.

Decisions for the author

• classify_accelerator_memory does not filter by CUDA_VISIBLE_DEVICES, only by HIP_VISIBLE_DEVICES/ROCR_VISIBLE_DEVICES — tradeoff, self-documented. device_visibility_mask_is_set's own doc comment (apps/rocm/src/diagnose_model.rs:149-158) states the HIP runtime honours CUDA_VISIBLE_DEVICES but the memory classification doesn't, so when only that variable is set, every amd-smi row is still treated as visible and the largest card on the box can be picked even if that exact card is the one hidden from the runtime. MASK_UNVERIFIED_NOTE already names CUDA_VISIBLE_DEVICES explicitly and warns the compared card "may be that of a different card" whenever a mask is set and GPUs differ in size, which is broad enough to cover this exact failure mode, not just a narrower "wrong card among a known set." This reads as a deliberate, mitigated v1 limitation rather than a silent regression — worth confirming that's the intended scope, since a real fix (actually deriving a CUDA_VISIBLE_DEVICES-filtered index set) is a bigger lift than a quick patch.
• Compute-buffer/runtime-reserve calibration still rests on one GPU class (MI300X) and one llama.cpp build — tradeoff, unchanged this round. #781's motivating cases are mostly low-VRAM consumer Radeon cards; the module doc, README and ESTIMATE_CAVEAT are all honest that another GPU or build can differ, so this isn't a defect, just worth reconfirming as the deliberate v1 scope now that the feature is close to merge.
• The duplicative wording when a model hits the new SSM/MoE "no verdict" path — non-blocking-improvement. The per-layout note pushed earlier (crates/rocm-core/src/gguf_fit.rs:727-741, e.g. "the recurrent state they keep is not counted...") and the new detail message (:795-799, "...so the total above can be too low") overlap in what they say without contradicting each other. Not a bug, just mildly redundant output; how notes vs detail compose on screen lives outside this file.

Positive signals

• ProjectorNamed's typed-error contract is honored end-to-end: raised at exactly one site (gguf_source.rs:276-281) and downcast at exactly two consumer sites (gguf_fit.rs, model_readiness.rs), verified by a repo-wide grep — no stray handling, no site coercing it into a generic error.
• The new header.ssm || header.moe "no verdict" early-return (gguf_fit.rs:785-800) is correctly sequenced after the figures (weights_gib/total_gib/available_gib) are populated but before max_context/suggestions run, so it can't leave a stale max-context or a suggestion that contradicts the "no verdict" outcome — confirmed by reading the full function top to bottom, and the pre-existing property-style sweep test's model builders were checked to not set any SSM/MoE metadata, so the new path can't silently interact with that sweep's own assertions.
• RelativeSeekBuf's claimed perf fix (a 300k-token local header: 342ms → 38ms) is backed by a real behavior-pinning test (a_local_header_is_skipped_through_without_seeking_the_file_per_token) using an instrumented reader that counts real seeks against a 50,000-token synthetic header — reverting the buffered relative-seek branch would drive the seek count far past the asserted threshold, not just leave a string assertion unchanged.
• main.rs/model_readiness.rs share one HostFacts read per diagnose invocation (explicit comment: "Read once: the memory classification and the mask caveat below both need the rows"), so the curated-model and GGUF-estimate paths consume one GPU-visibility classification and cannot disagree about it by construction.
• Every new CLI-observable behaviour added since bd8015e3 (projector-by-filename refusal and naming, context-option refusal as a usage error, curated-model context-option no-op, missing-path handling, bare-repo-with-context-size, projector-marked listing) got a new Gherkin e2e scenario in the same round (diagnose-51 through diagnose-56), not deferred to unit tests alone.
• API surface was tightened correctly: LiveGgufSource::with_hub and RangedReader::fetched moved to #[cfg(test)]-gated pub, confirmed via a repo-wide grep that no production caller depended on either (previously any rocm-core consumer could redirect hub requests to an arbitrary host via with_hub).

Deployment notes

• Any non-catalog owner/repo:tag passed to --model triggers a Hugging Face network request by default (listing, possibly HEAD and range requests), documented in --help and the README; unchanged this round.
• No migration or new required configuration.

What this covered

Full diff bd8015e3...cf654f9c (13 files, +1658/-126) reviewed via four independent fan-out workers grouped by coupling: (1) gguf.rs + gguf_source.rs — the latter read whole and fresh (1972 lines) since it carried an open finding; (2) gguf_fit.rs alone, read whole and fresh (3481 lines), also carrying an open finding; (3) apps/rocm/src/diagnose_model.rs + crates/rocm-core/src/model_readiness.rs, both read whole and fresh, plus apps/rocm/src/main.rs's small diff with surrounding context (the 9722-line file itself was not re-read in full, as nothing in it carried an open finding and its delta here was ~13 lines); (4) docs (README.md, docs/testing.md, docs/manual-testing.md, skills/rocm-doctor/reference.md) + e2e (diagnose.feature, diagnose_steps.rs, e2e.rs). Each worker reported independently and blind to the others and to this PR's review discussion; none failed, none needed respawning, none needed to split further. I independently re-verified the two highest-value items myself afterward by re-reading gguf_source.rs:224-275 and the apply_context_estimate/read_gpu_memory_fix branching in model_readiness.rs:855-884, and directly compared gguf_fit.rs:795-799 against :1040 to confirm the epsilon-omission finding, rather than trusting any worker's framing. Files unchanged since bd8015e3 (net.rs, lib.rs, docs/architecture.md) were not re-read; the prior round covered them completely and carried no open finding against them.

Did not run as dedicated passes: history (git log/git blame) — folded into each worker's per-file read instead of a separate blame sweep. prior-changes — this method has no access to review discussion, so "do earlier review comments apply here" could not be checked; note the mandatory claim-check at the start of this process (confirming no review already exists at this exact head) necessarily surfaced prior review metadata and body text as an unavoidable side effect of that administrative check — that text was not used as input to any finding above, and the two previously-open findings re-confirmed here were independently re-derived from source, not quoted from it. Code-comments was folded into each worker's file read. CI: all 26 checks green at commit cf654f9c (gh pr view --json statusCheckRollup), no failing or pending checks. Issue #781 retrieved directly (title/body/state only); not reconciled against any discussion on PR #826 itself.

---

🤖 Generated with Claude Code

[tomastola]

@nowycondro, replying to your automated review at cf654f9:

The MiB rounding: fixed in 3b6b9f3. Both lines that show the host's memory in MiB now go through one helper, whole_mib_offered, which keeps the 1e-6. The new test the_no_verdict_detail_shows_the_same_mib_as_the_evidence_line uses a figure a hair under 8 GiB and checks that both lines show 8192 MiB; with the plain floor put back, the detail shows 8191 and the test fails. As you say, no real source triggers it today: amd-smi MiB and decimal-MB conversions floor to the same figure both ways.

On the decisions:

• CUDA_VISIBLE_DEVICES: deliberate for this PR. The mask caveat names it and covers the wrong-card case. Working out which card that variable leaves visible is a separate change.
• Calibration on one GPU class and one llama.cpp build: deliberate v1 scope, and stated in the module docs, the README and the caveat on every estimate. Measurements on consumer Radeon cards would be the next step to tighten it.
• Note vs. detail wording on the no-verdict path: leaving it as is. The note says what is not counted, and the detail turns that into the consequence (no verdict, the total can be too low). The note also says things the detail does not (for state-space layers, the KV-cache part can be lower), and it is shown on the unknown-memory path too, where there is no such detail.

[jussielo-amd]

Requesting changes, mainly on three points where the PR claims something its code or tests do not back. The ten branches I listed in my earlier review now have tests, and the single predicate behind the verdict, the largest context and the suggestions is a good design. This pass is at 3b6b9f3 and is a read-through: I did not run the tests or mutations, and the MI300X and W7900 lanes had not reported yet.

Blocking

1. A header missing the embedding or feed-forward width still gets a fits / does-not-fit verdict, which contradicts "No verdict where the figure can be too low". gguf_fit.rs:580-585 turns the missing width into 0, and :743-749 then says the compute buffer is "understated and the real figure can be higher". The code goes on to decide Fits or DoesNotFit, and a_header_without_the_widths_the_compute_buffer_needs_says_it_is_understated (:2096) asserts Fits. The note and the verdict disagree, and the rule in the description and in the comment at :785 should govern. Either return MissingMetadata from sizing(), as is done for a missing head or layer count, or state this as a deliberate exception.

2. "No token is sent" is asserted nowhere. It is in --help, the README, the error text at gguf_source.rs:578 and the description. The stand-in hubs record path and Range but no headers, and a_gated_file_is_said_to_be_unreadable_without_offering_a_token pins the string only. This is a promise that something will not happen, so it needs the behaviour asserted with it: record request headers and assert no Authorization or Cookie on the listing, HEAD and range requests, with HF_TOKEN set in the child environment.

3. Branches this PR adds have no test.

• gguf.rs:361-378, the shared fixed_width table: signed_values_of_every_width_decode_by_their_own_sign_bit covers type ids 0, 1, 2, 3, 5 and 11. Scalar bool (7, the type of tokenizer.ggml.add_bos_token), u64 (10) and f64 (12) are never parsed, and most element types are never parsed as array elements. A wrong width on any row desyncs every later key. A table-driven test over ids 0–12, as scalar and as array element, each followed by a kept sentinel key, would cover it.
• gguf_source.rs:147-155, has_label: no fixture has a letter before the label, such as BF16 beside F16, so the before-boundary can be dropped without a failure.
• gguf_source.rs:779-786 and :699-709: a 206 with no Content-Range, and a HEAD with no Content-Length. Every response in the fake hub carries both.

Non-blocking

• Scenarios. diagnose-52 to -56 cover a lot of the new behaviour. Still without one: does_not_fit with its suggestions and largest-context line, the default-context text, the refused layouts in JSON (unmeasured_layout, split_file, latent_attention, no_attention_layers, missing_metadata) and not_a_model in JSON. The description argues only that does_not_fit depends on the host, and the layouts are reachable with a hand-built local fixture on any lane. Separately, the compared half of diagnose-48 and -49 accepts either fits or does_not_fit, so a swapped predicate stays green at the e2e level.
• Wrong card under ROCR_VISIBLE_DEVICES. usable_amd_gpu_indices_from re-indexes ROCR survivors to 0..N, while classify_accelerator_memory (main.rs:3108-3113) matches them against amd-smi's usage.index. If that index is the physical ordinal, ROCR_VISIBLE_DEVICES=1 on a 24 GiB plus 8 GiB host compares the 24 GiB card, and the header can say "ESTIMATED: fits" with the doubt only in the note below it. I read the re-indexing but did not check amd-smi's index. Consider giving the figures with no verdict when a mask is set and the rows differ.
• shard_parts (gguf_source.rs:312) uses the first -of-, so Chain-of-Thought-…-00001-of-00003.gguf returns None and reports "matches several files". rfind fixes it.
• Remediation commands. The verify: lines (model_readiness.rs:888, :909) are rocm diagnose --model {ref}: they drop --context-size and --kv-cache-type, and a local path with a space is not quoted. The description says a GGUF estimate with no GPU still has no plan, but the curated no-GPU answer offers rocm diagnose.
• Text that contradicts the code.
  • gguf_fit.rs:27-32 still says mixture-of-experts and state-space models "can miss by more", but the code now withholds their verdict.
  • gguf_fit.rs:999-1001: the doc comment and #[must_use] for evidence_lines sit on whole_mib_offered.
  • model_readiness.rs:707 says other answers are "left exactly as it was", but LeaveAsIs adds an evidence line.
  • The test comment at gguf_fit.rs:3119 says 40% where weights: 400 of data: 1000 is 60%.
  • docs/manual-testing.md:932 says "about 15.8 GiB", but the baselines in the same paragraph give 15854 MiB (15.5 GiB) and 15500 MiB (15.1 GiB).

Questions

• #781 item 3 asks for partial CPU offload as a suggestion, so I take it as intended. docs/llm-tool-use.md still says the assistant "must never suggest CPU fallback" and could relay this output, so please confirm that is the plan.
• "Fits" is decided at the point estimate against total VRAM, with no margin, and the header reads an unqualified "ESTIMATED: fits". The free-memory caveat is only in an evidence line. Is that deliberate?
• Does llama.cpp pad the cache to a multiple of 256 under flash attention? If so, a request that is not a multiple of 256 is under-priced by up to 255 tokens, and every calibration load is a multiple of 256. I did not confirm this. Likewise, the smaller-quantization suggestion accepts "fits" with zero margin on an averaged bits-per-weight table; is that intended?

[juhovainio]

I went through this against the issue's steps 1-3 and the parser, estimator and docs look careful. The verdict, the largest-context figure and every suggestion come from one predicate, and the hostile-input bounds on the parser and the remote reader are good. There is one correctness concern I'd like fixed before merge, plus a few smaller points.

Blocking

• The estimate removes the whole token_embd.weight from the VRAM weights. For tied-embedding models (no output.weight, e.g. the Gemma and small Llama/Qwen/Phi families) llama.cpp duplicates that tensor onto the output-layer device, so it does take VRAM. For those models the estimate comes out too low, in the unsafe direction, and can say "fits" when it doesn't. The calibration described in the module docs used two untied models, so it doesn't cover this case. I've argued this from llama.cpp's loader behaviour and haven't run a tied model here. Details in the inline comments.

Non-blocking

• The evidence_lines doc comment and #[must_use] ended up above whole_mib_offered (inline).
• No test exercises a successful remote read end to end. On the real hub resolve/main answers with a redirect to a CDN, and fill() requires a 206 with a matching Content-Range after following it. Could you confirm with one manual run against huggingface.co that this holds, and that the MI300X run in the manual-testing notes covered a remote ref?
• The PR bundles several independent behaviour changes (the device-mask Dedicated(0.0) fix, the --report skip, projector handling). They are all described in the body, but each would be easier to review and revert on its own.
• MoE and hybrid-SSM models get figures but no verdict. That is honest, but it covers a lot of current models, so it is worth stating plainly in the docs.

CI is green apart from the Strix Halo Ubuntu lanes, where the step was cancelled at about 45 minutes (looks like a runner timeout), and W7900, which was still pending when I looked.

[juhovainio]

The whole token_embd.weight span is removed from VRAM weights. For a tied model (no output.weight) llama.cpp duplicates this tensor onto the output-layer device, so it does occupy VRAM; the estimate is then too low in the unsafe direction, and the calibration (two untied models) doesn't cover it. Please keep the embedding in the weights when output.weight is absent, add a test with a tied header, and say in the docs/README which model shapes were calibrated.

[tomastola]

Fixed in 93eef14. The input embedding is now left out of the VRAM weights only when the file has its own output.weight. A tied header keeps it in. the_input_embedding_is_only_left_out_of_the_weights_when_the_header_has_one now has a tied case, and it fails if the rule is undone.

One thing the calibration data shows: the Qwen2.5-1.5B load I calibrated against is itself tied. In llama.cpp's log, its GPU model buffer was the tensor data less one embedding (1564.63 MiB), with no copy. The old estimate came out 0.05% to 2.5% above its measured loads. But the overhead left over for that model was about 180 MiB more than for the Llama-shaped one, and I can't rule out that the output layer's use of the embedding sits there. So I took the side that can't say "fits" wrongly. With the embedding counted, the Qwen-shaped estimates are now 3.6% to 9.5% above its loads, and the calibration test allows 10% for it. The module docs, the README and docs/testing.md now name the two calibrated shapes (Llama-3.1-8B-shaped with its own output layer, Qwen2.5-1.5B-shaped with tied embeddings) and give the new margins.

[tomastola]

A correction to my reply above: the calibration model was not tied. jussielo-amd pointed out, and I've confirmed against the real Q8_0 file, that Qwen2.5-1.5B-Instruct-GGUF carries its own output.weight. My fixture had left it out. So the "180 MiB unexplained overhead" argument and the 3.6% to 9.5% figures came from the fixture, not from llama.cpp.

The rule you asked for stands: with no output.weight, the embedding is counted on the GPU. It is now documented as unmeasured, since both calibration files have their own output layer, and it can only overstate. With the fixture fixed (7b45256), the calibration is back to 0.05% to 5.5% above all sixteen loads.

[juhovainio]

Only token_embd.weight is recorded; whether output.weight exists is never captured, which is what sizing needs to decide whether the embedding really stays on the host.

[tomastola]

The parser now records this as GgufHeader::has_output (93eef14), with a test that a tensor listed after output.weight doesn't clear it (6e362f8). sizing uses it as described in the other thread. The Llama-shaped test fixture now carries its own output.weight, as Llama 3 does.

[juhovainio]

The evidence_lines doc comment and #[must_use] sit above whole_mib_offered, so evidence_lines is undocumented and whole_mib_offered carries the wrong docs. Move whole_mib_offered above them.

[tomastola]

Fixed in 5a83976. whole_mib_offered now sits above the evidence_lines doc comment and #[must_use], so each has its own docs.

[tomastola]

Rebased onto main (435fbcc) and force-pushed; the branch is at 4f07c90.

• refactor(rocm): move the clap command tree into cli.rs (proposal) #889 moved the clap command tree into cli.rs. This PR's changes to it went with it: the --context-size / --kv-cache-type flags, the --help example and the parse tests, including serve_takes_no_estimate_options_yet. The flag types stay in diagnose_model.rs, and the dispatch stays in main.rs.
• Every commit now compiles with its tests. The "follow main's move of the diagnose command" fix is folded into the commit that needed it, so there are 23 commits. All are signed and carry a sign-off.

Run locally on the rebased branch: fmt, clippy, xtask, the unit tests, the no-token test and the GGUF e2e scenarios.

• storage::tests::prune_removes_the_install_it_named_and_not_a_case_twin_of_it fails, as it does on main here.
• Three process::tests timing tests failed under a loaded full run and pass on their own. This PR doesn't touch process.rs.