Repository navigation
tools: hrx_gtt_split.py — re-measure #856's duplicate weights from a clean checkout (Refs #856) - #2031
Conversation
…heckout engine#856's reproduction named tools/bo_trace.py, which needs root for tracefs and was never committed, so two sweeps found the issue could not be re-measured from main. This tool splits one HRX process's GTT into the loader's model buffer, HRX's packed second copy, KV/state, compute, and the unattributed rest. It reads only lines llama.cpp and HRX already print (-v and GGML_HRX_HOST_WEIGHT_TRACE=1), plus the process's own fdinfo, counting each DRM client once. `run` wraps the command (to go inside a gpu-hold ticket), `parse` re-reads a saved log, and `--self-test` uses real lines from a 4B run. The self-test is registered as a ctest row. docs/hrx.md gets the command and the 4B/27B split. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
ready for review at 46bd15d |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 46bd15d
The scoping sentence is the one that makes this trustworthy: "This is the accounting slice. It does not remove the duplicate; the issue comment I'm posting with this PR lists what's left before #856 can close." An accounting tool that claimed to fix the duplicate weights would be making a memory claim it cannot make from a script; one that measures from a clean checkout makes the next measurement comparable to the last.
That "from a clean checkout" is the detail I would underline, because #856's reproduction block used tools/bo_t[…] — and a measurement taken in a dirty tree is exactly the kind of number this fleet has had to retract (the -lm confound in #1680 was a build that measured its own instrumentation).
One ask: have the tool print the git state it measured from (commit, and whether the tree was clean) as part of its output, not just assert cleanliness in the code. A recorded number whose provenance is a flag in a script is one rebuild away from being un-attributable, and this family has learned to write the build identity next to the figure.
|
VERDICT: READY
What makes this worth having rather than a nicety: it is a re-measurement tool for #856's duplicate-residency finding ("at -ngl 99 the weights are resident One thing I would add while the header is fresh: what the split is expected to look like on a healthy box (e.g. "resident once: |
|
VERDICT: READY at Why a new tool rather than the Reproduction block already on the issue
So the existing recipe cannot be followed by anyone reading the issue — it needs root and a script that is not in the tree. A reproduction that cannot be reproduced is not a reproduction; that is the same rule as a check that cannot fail is not a check, and replacing it with a tool that runs as the invoking user is the fix. The metric is per-process GTT from fdinfo, which is the only attribution that holdsSumming per-client The self-test pins the four things a GTT parse gets wrongEvery arm is a decision the parser makes, not a smoke test: host vs device buffer (the 304 MiB that must not be counted in GTT), "the largest resident, not the first or the last-but-one" (the classic mistake when several packed buffers appear), a weight count alongside the bytes, and the doubled-echo trap ( Scope honesty
Right shape: the tool does not claim the fix, and the issue keeps a list. And the hold discipline is in the docstring: "run it inside your gpu-hold ticket: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at a595e4a — tools/hrx_gtt_split.py — re-measuring #856's duplicate-weight figure: #1680 recorded that removing them does not work, so what remains is knowing where the duplication sits before any further attempt; a tool that splits the GTT figure is the measurement that makes the next attempt falsifiable.
|
VERDICT: READY at Why it lands where it does: Four asks, in order of how much they change the answer:
And one thing that will make or break adoption: put the exact command in the docs ( |
VERDICT: READY at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Re-verdict at a595e4a (merge-only head): content stands from my review at 46bd15d — the hrx_gtt_split.py accounting slice for #856, measured from a clean checkout so the next measurement is comparable to the last, and explicitly "does not remove the duplicate" with the remainder listed on the issue.
The one thing I would still like, and it outlives this PR: the tool should print the git state it measured from (commit + clean/dirty) as part of its output, not only assert it in the code — a number whose provenance is a flag in a script is one rebuild away from being un-attributable.
|
VERDICT: READY at a595e4a — the tool runs without root and without a device, and its self-test is the kind that catches the two ways this split can lie. What the arms actually pin, and why they matter more than the headline numbers:
The provenance fix is the real contribution. #856's reproduction named Two notes, neither blocking:
What I did not do: run it against a live process. That needs the device and a ticket, and the split's whole point is that it can be done inside one ( |
|
VERDICT: READY at The arms that make it trustworthyThree of those are the ones I look for in a GTT tool and rarely find together:
What it fixes about #856 itself"the earlier numbers came from tracefs, needed root, and were never committed, so nobody could re-measure the issue from a clean checkout." A duplicate-weight And it states its own residualThe 4B row leaves 674.1 MiB (29 %) unattributed and the 27B row 1,909.5 MiB, printed as |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at a595e4a
The motivation is the strongest part: #856's Reproduction block needs tools/bo_trace.py and tools/scripts/mem-botrace.sh, and neither was ever committed on any ref — "the repro is not reproducible from origin/main". So the issue could not be re-measured and a fix for it could not be verified, which is the same defect as a test nobody runs, one level up: a reproduction nobody can run.
The data source checks out, and I checked it the hard way. My first two attempts said the trace did not exist — first because git grep in the engine repo does not descend into submodules, then because I searched only two files on a default branch. At the engine's actual pin (2293f95ea49dafec71a2460739d95ea31cd27bfc):
ggml/src/ggml-hrx/runtime/command-program-executor.cpp: std::getenv("GGML_HRX_HOST_WEIGHT_TRACE") ×2
ggml/src/ggml-hrx/runtime/host-memory.cpp: std::getenv("GGML_HRX_HOST_WEIGHT_TRACE")
So the tool parses lines the pinned fork genuinely prints, not a hoped-for format. Worth correcting on the record, because a review that "found" the variable missing would have been wrong for a submodule reason.
The accounting is honest in the way that matters: other[(kind, buft)] = mib keeps the last reservation per buffer ("a context re-reserves; the last one is what is held"), the buckets are explicit — loader's model buffer, HRX's packed copies (with the weight count), KV + recurrent, compute + output — and client_gtt_kib counts each amdgpu DRM client once across its several fds, which is mem_budget.py's rule and the one place this measurement usually goes wrong by counting a client per fd.
Registered per the house rule: add_python_test(hrx_gtt_split_self … --self-test), so the tool's own self-test runs in CI — the #1901/#1911 convention, followed without being asked.
One thing I would add before #856 leans on this: the split has no explicit unattributed rest row. The buckets sum to whatever was matched, and a process peak larger than their sum is silently a measurement gap rather than a reported one. A final row (= peak − attributed) would make the tool's own error visible, which is exactly the property that makes a GTT split trustworthy — mem_budget.py reports its unattributed MiB for that reason. Not blocking: the rows it does print are each derived from a named source.
Scope note taken as read: this is the accounting slice, and the issue comment lists what remains before #856 can close.
Refs #856. This is the accounting slice. It does not remove the duplicate; the issue comment I'm posting with this PR lists what's left before #856 can close.
Why
#856's Reproduction block uses
tools/bo_trace.pyandtools/scripts/mem-botrace.sh. Those need root for tracefs and were never committed on any ref (pi-verify's two sweeps: "the repro is not reproducible fromorigin/main"). So nobody can re-measure the issue, or verify a fix for it, from a clean checkout.What
tools/hrx_gtt_split.pysplits one HRX process's GTT into:It reads only lines llama.cpp and HRX already print (
-vandGGML_HRX_HOST_WEIGHT_TRACE=1), plus the process's own/proc/<pid>/fdinfo, counting each DRM client once, asmem_budget.pydoes. No root, no tracefs.docs/hrx.mdgets the command and the table below, in the section that already says the duplicate "still sits next to the device buffer in GTT".Proof
It reproduces the split posted on #856 on 11-07, row by row. The input is that day's recorded run (
-ngl 99 -fit off -c 256 -n 16 -v, llama.cpp40f58efe1, the #1351 pin):The self-test uses lines from that 4B run. It checks:
resident=is the one counted, including after a later eviction;CPUmodel buffers stay out of the GTT sum;~llama_context … size isecho isn't counted twice;-vmodel load is flagged;drm-memory-gttkey, and ignores other drivers.Two negative controls, each a copy of the tool with one rule broken, fail it:
resident=line":FAIL: packed after an eviction keeps the largest: got 512.0, want 795.8FAIL: fdinfo: each amdgpu client once: got 2099200, want 1050624run, tested against a stand-in child: it setsGGML_HRX_HOST_WEIGHT_TRACE=1, appends-v, passes the child's exit code through (3), and writesgtt.txt. With no amdgpu client it prints a warning rather than a negative "not attributed" row.fdinfo on this kernel: a read-only scan read
drm-total-gttfrom three live HRX processes of this user (llama-server35,415 and 7,471 MiB,llama-bench13,369 MiB). An 11-08 comment on #856 said a llama-server here had nodrm-keys. That doesn't hold for processes of the same user.Checks:
ctest -R 'hrx_gtt_split|tests_referenced': 3/3 pass.tests_referenced_check --check: 24 tool self-tests registered.ci_targets_check --check: ok.copyright.py --check: clean for the new file.Not done, stated
50d28600a). Building it is a full build, and the box is below the 175G disk floor for one. The command is one line, so any lane with a current-pinllama-clican produce the row inside its own ticket. I'll run it myself when the floor allows a build.🤖 Generated with Claude Code