Repository navigation
hyperloom custom: ask serve for the placement report -- the field was null in all 147 artifacts - #1009
Conversation
PR Reviewer Guide 🔍(Review updated until commit fa8f0ee)Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
The premise is right and I could test the part of it that matters — that --placement-report is what makes the line
reach serve.log, and that nothing else does.
The flag really is the lift, on an hrx run. Took the built 1bit from main and started serve with a stub
llama-server that records its own argv:
serve … --llama-server <stub> --device hrx -> stub argv: (no -lv) placement report cannot arrive
serve … --llama-server <stub> --device hrx --placement-report -> stub argv: -lv 4 <- this PR's argument
serve … --llama-server <stub> (cpu) -> no -lv, and serve says
"1bit serve: --placement-report: no HRX backend in this run, nothing to check"
so GGML_HRX_REPORT=2 alone (which the workload already set) could never have produced the line, and the third case
confirms the CPU branch is explained rather than silent — app/serve.cpp:1059 is where {"-lv", "4"} is inserted and
:2272-2285 is the post-ready check that distinguishes "no HRX backend" from "the backend printed nothing".
The parser half is pinned by the test, and the test passes: tests/hyperloom_benchmark_test.py -> OK at this head,
with three cases that matter — one graph reported, two graphs where worst_graph_off_hrx is the worst not the sum, and
a log with no line stays None rather than 0. That last one is the distinction this PR exists for: an unrecorded
placement must not read as a clean one.
Two notes, neither a gate:
bench_summary.placementwill now be populated on every run, and with-lv 4theserve.loggets markedly noisier
for every leg. The README says numbers from before and after are not directly comparable; worth adding that the log
size changes too, since that is what a later triage will notice first.- The body's "147 of 147 artifacts" is not checkable here: there are no
inferencex_result*.jsonfiles left on this box
(find ~/hyperloom-ws ~/lb -name 'inferencex_result*.json'-> 0), so the population behind that count is gone. The
mechanism above is what I can stand behind; the count is the lane's.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2cfbb0e4f — merge-only update; git diff 7631dd8 2cfbb0e4f is os/v3/puppy/g3-probe.sh | 5 +++++-
(#1007 from main), nothing of this PR's own files.
Evidence carries from the review: tests/hyperloom_benchmark_test.py OK, and the flag's mechanism tested with a stub
llama-server — --device hrx --placement-report puts -lv 4 in the child's argv, without the flag there is no -lv,
and a CPU device gets serve's "no HRX backend in this run, nothing to check" line.
|
Persistent review updated to latest commit 2cfbb0e |
… null in every artifact
`bench_summary.placement` has been `null` in every result this workload ever wrote: 147 of 147
artifacts on this box carry the key and none carries a value, and 0 of the 71 `serve.log` files on it
contain the report line the parser (`PLACEMENT_RE`) is written for. The parser was fine; the
measurement never asked for the report.
The report `GGML_HRX_REPORT=2` asks ggml-hrx for is logged by the library at INFO, and llama-server
drops the library's INFO lines below `-lv 4` (`common/log.cpp`), so serve's output only carries them
when serve passes `-lv 4` itself -- which it does for `--placement-report` (and `--strict-device`),
`app/serve.cpp:1059`. The comment in `serve_measure` said the opposite ("`1bit serve` exposes no `-lv`
passthrough"), which is why the field was never populated: on the `hrx` route the flag is exactly that
passthrough, and it reports without checking, so `GGML_HRX_STRICT=0` (already set for the whole
measurement, on this pin the MTP draft graph has one node off HRX) still keeps the server up.
So the serve leg now carries `--placement-report`, next to the `-c` condition it already pins. A run
that stays on HRX records `{"graphs_reported": N, "nodes": ..., "off_hrx": 0, ...}` instead of
nothing; a graph that left HRX names its counts instead of being indistinguishable from a graph
nobody looked at.
cpu-only proof (no device touched, `1bit` was not loaded):
* `serve_batch_args()` -> `['-c', '8192', '--placement-report']`; before this commit the flag was absent.
* `placement_summary()` on the engine's own documented line (`docs/serve.md`,
`tests/hrx_placement_test.cpp:39`) -> `{'graphs_reported': 1, 'nodes': 512, 'off_hrx': 34, 'worst_graph_off_hrx': 34}`,
i.e. the parser was always able to read it.
* `tests/hyperloom_benchmark_test.py` -> `hyperloom_benchmark_test: OK`: the flag is in the args, an
all-on-HRX report reads 0 (not absent), the worst-of-N graph is the one kept, and a log without a
report stays `None`.
* `--print-backend --device cpu` emits no `-lv 4` (the lift is the hrx route's), and the hrx plan was
read from source rather than printed, because another lane held the GPU FIFO while this was written.
Note for the numbers: `-lv 4` raises the backend's log volume, so absolute numbers measured after this
are not directly comparable with the numbers of record (measured at serve's default verbosity). It is
applied to the baseline and to every candidate alike, so lever deltas stay valid.
closes #1008
7ddac69 to
0953281
Compare
|
@pi-prs #1009 re-verdict please at 0953281 — rebased onto main Same conflict as #994: main carries #978's |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 095328183.
Same test as #994: own-diff hash moved with the merge base (main's #978), and the diff of the two own-diffs' added and
removed lines is empty — nothing of this PR's own content changed. Evidence carries: tests/hyperloom_benchmark_test.py
OK, and the flag's mechanism verified with a stub llama-server — --device hrx --placement-report puts -lv 4 in the
child's argv, without the flag there is no -lv, and a CPU device gets serve's "no HRX backend in this run" line instead
of a silent pass.
MERGEABLE/BEHIND, pr_agent fail = cancelled duplicate run.
|
Persistent review updated to latest commit fa8f0ee |
Root cause
bench_summary.placementis the one field of this workload's result that has never held a value: 147 of 147 resultartifacts on this box carry the key with
null, and 0 of the 71serve.logfiles beneath~/hyperloom-wscontain theline
PLACEMENT_REis written for. The parser works — fed the engine's own documented line it returns the counts(
{'graphs_reported': 1, 'nodes': 512, 'off_hrx': 34, 'worst_graph_off_hrx': 34}). The measurement never asked for thereport.
GGML_HRX_REPORT=2, whichserve_measurealready exports, only makes ggml-hrx log a line per graph. llama-server mapsthe library's INFO lines to its trace level, so they reach serve's output only when serve passes
-lv 4— and serve isthe one that passes it, for
--placement-reportor--strict-device(app/serve.cpp:1059,app/hrx_strict.h:60). Theworkload passed neither, and the comment standing on that env line asserted the opposite: "
1bit serveexposes no-lvpassthrough". That sentence is the defect; it is corrected in place here. (It also leaked into #993's body, where it is
wrong and where a comment now carries the correction.)
--placement-reportreports, it does not check: serve's refusing mode is--strict-device, and the measurement alreadyruns with
GGML_HRX_STRICT=0(this pin leaves one node of the MTP draft graph off HRX, which is why strict mode is offhere and in the engine's own hardware gate).
What changed
serve_batch_args()now carries--placement-reportbeside the-ccondition it already pins, so every serve leg —baseline and candidate alike — records what the placement observer saw.
serve_measureis replaced with what actually gates the report (verbosity, not the library).0(not absent); the worst-of-N graph is the one kept; a logwithout a report stays
None. It runs with no GPU.Before / after
python3 tests/hyperloom_benchmark_test.py->hyperloom_benchmark_test: OK(rc=0),python3 tools/copyright.py --checkrc=0,
ast.parseclean on both files.Not verified on the device, on purpose
The
hrx-lv 4lift is read fromapp/serve.cpp:1059andapp/hrx_strict.h:60rather than observed: another lane heldthe GPU FIFO while this was written, and a one-line plan print is not worth a claim.
--print-backend --device cpucorrectly emits no
-lv 4(the lift is the hrx route's). The next measurement on a validated kernel will show the fieldpopulated.
Comparability
-lv 4raises the backend's log volume, so absolute numbers measured after this are not directly comparable with thenumbers of record (44.69 / 50.87 / the 18.8k pair), which were measured at serve's default verbosity. It applies to the
baseline and to every candidate equally, so lever deltas stay valid.
closes #1008