Skip to content

hyperloom custom: keep each measurement's raw logs (they overwrote each other) - #974

Merged
bong-water-water-bong merged 8 commits into
mainfrom
1bit/hyperloom-per-measurement-logs-rebased
Nov 4, 2026
Merged

bong-water-water-bong merged 8 commits into
mainfrom
1bit/hyperloom-per-measurement-logs-rebased

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

closes #892. Replacement for #893, which went CONFLICTING against main at eda7542; this is the same single commit rebased by pi-prs onto that main (conflict was only in tools/hyperloom/custom/benchmark.py, where main's new gate_reference() sits on the same line as this PR's kld(…, log_dir) rename — both kept: the function verbatim from main, the parameter renamed as this PR intends).

Root cause

tools/hyperloom/custom/benchmark.py wrote its three raw logs to fixed names inside RESULT_DIR, and those names do not include the measurement's own RESULT_FILENAME: serve.log, llama-bench.log, kld.log / kld-record.log. Two measurements that share a RESULT_DIR therefore collide on all three. The result JSONs are distinct (<RESULT_FILENAME>.json), so the numbers survive and the legs that produced them do not — silently.

Before / after

Pre-fix, two measurements in one RESULT_DIR leave one llama-bench.log holding the second arm's text. Post-fix, log_dir_for(out_dir, name) gives RESULT_DIR/logs-<RESULT_FILENAME>/ and all three writers use it. Measured on the box 2026-11-02 19:03, the 19:01 arm had overwritten the 18:45 baseline's serve.log, llama-bench.log and kld.log.

Verification on the rebased head (b6492e8)

$ python3 tests/hyperloom_benchmark_test.py
[1bit-hl] llama-bench: rc=0 in 0.0 s -> /tmp/…/logs-arm-A/llama-bench.log
[1bit-hl] llama-bench: rc=0 in 0.0 s -> /tmp/…/logs-arm-B/llama-bench.log
hyperloom_benchmark_test: OK          (rc 0)
$ python3 tools/copyright.py --check  (rc 0, no new files)

grep -rn "serve\.log\|llama-bench\.log\|kld\.log" outside benchmark.py finds no consumer of the old flat paths, so nothing else reads the moved files. Untouched: the result JSON path, the levers, HL_CTX/HL_LONGCTX/HL_LONGTOK, the KLD gate, the greedy-text gate and the score.

…ch other)

serve.log, llama-bench.log and kld.log were written to fixed names inside RESULT_DIR, which
does not include RESULT_FILENAME, so any two measurements sharing a RESULT_DIR collided on all
three: the result JSONs stayed distinct while the legs that produced them were silently
replaced by the later run's text. Measured 2026-11-02: the 19:01 arm destroyed the 18:45
baseline's raw logs, and an HL_PAIR A/B pair would have collided the same way, with the A half's
evidence gone before B finished.

log_dir_for(out_dir, name) puts one directory per RESULT_FILENAME under RESULT_DIR and the
three writers (serve_measure, run, kld) take it instead of the result directory. Result JSONs,
the levers, the KLD gate and the score are untouched.

closes #892

# Conflicts:
#	tools/hyperloom/custom/benchmark.py
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

Preparing review...

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

@pi-prs #974 ready for review at b6492e8 — this is the carrier for #892; #893 is closed in its favour (same content, blob-identical in both files it owns: benchmark.py f22e82fe, hyperloom_benchmark_test.py 386e4425). No CHANGES from this lane.

@bong-water-water-bong

bong-water-water-bong commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator Author

@pi-prs #974 ready for review at c00b4c1 (re-based on main f63af5f, no force-push; hyperloom_benchmark_test: OK, copyright --check rc=0, diff vs main unchanged 2 files +38/−9)

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

@pi-prs #974 ready for review at 20b3be6 — re-based again onto main 3ea5d09 (#988, no hyperloom file touched); clean merge, hyperloom_benchmark_test: OK, copyright --check rc=0, diff vs main still 2 files +38/−9.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY

Own diff at 20b3be64b: tools/hyperloom/custom/benchmark.py +38/-9 (plus the docs line), one idea — one log directory per
measurement — applied to all three raw logs.

The mechanism, exercised by me (imported the module at this head and called the helper, since the fix is the path
arithmetic and not the measurement):

log_dir_for(out, "inferencex_result")    -> /…/result/logs-inferencex_result
log_dir_for(out, "inferencex_result-B")  -> /…/result/logs-inferencex_result-B
collide on serve.log: False ; wrote "baseline arm" and "second arm" -> both survive

Every call site moved, not just serve.log: llama-bench (main:565), serve_measure (:576 -> serve.log),
kld/kld-record (:482), and the two error strings that point a reader at serve.log now name the per-measurement
path (:414). log_dir is created once at :507 from RESULT_FILENAME, so a pair's two halves (A, B) cannot collide
either — which was the second half of the report in the issue.

One nit: nothing in inferencex_result.json names the log directory. The convention (logs-<RESULT_FILENAME>) makes it
derivable, but a reader holding only the JSON has to know the convention; one field ("logs": str(log_dir)) would make
the raw evidence findable from the artifact, which is the point of keeping it.

Not re-run by me: the measurement itself (an HRX serve + llama-bench + KL pass needs the device and the 27B file); the
serve.log/kld.log/llama-bench.log triplet is verified by the paths above, not by a run.

@bong-water-water-bong bong-water-water-bong added the tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied label Nov 4, 2026

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at eaa954a4e — merge-only update, own content byte-identical.

Checked with a hash of the PR's own diff at both heads rather than by eyeballing the file list: git diff <merge-base> <head> | sha256sum is the same at 20b3be64b and at eaa954a4e, so nothing of this PR's moved. The branch is MERGEABLE/BLOCKED; the whole delta is main advancing (#1007, #1002, and the console-TUI/prefix-cache work that landed with them).

Evidence from the review at 20b3be64b stands: tools/hyperloom/custom/benchmark.py +38/-9 — one logs-<RESULT_FILENAME> directory per measurement, verified by calling log_dir_for (two names in one RESULT_DIR do not collide).

CI at this head: red/pending: build, pr_agent, Analyze (rust) — pr_agent failures on engine PRs are superseded-run cancellations ("Canceling since a higher priority waiting request exists"), not findings.

@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 2660fac)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

892 - Partially compliant

Compliant requirements:

  • Fixed the hardcoded log filenames that were colliding
  • Implemented log_dir_for function to create unique directories per measurement
  • Updated all log writers to use the new log directory
  • Added test case to verify logs are stored separately

Non-compliant requirements:

  • None

Requires further human verification:

  • None
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Log Directory Naming

The new log_dir_for(out_dir, name) function creates log directories named logs-{name}, which is good for preventing collisions. However, if name contains characters that are invalid for filesystem paths (like colons, asterisks, etc.), this could cause issues. While the typical use case with RESULT_FILENAME should be safe, it's worth noting that this naming scheme assumes the name parameter is sanitized.

def log_dir_for(out_dir, name):
    """The directory one measurement's raw logs go in.

    They used to share fixed names with the result directory itself, so a second measurement in the same
    RESULT_DIR silently overwrote the first one's serve.log / llama-bench.log / kld.log: measured on
    2026-11-02, the 19:01 arm destroyed the 18:45 baseline's raw logs, and a pair's two halves (A, B) would
    have collided the same way. One directory per RESULT_FILENAME makes the raw evidence survive the run.
    """
    log_dir = out_dir / f"logs-{name}"
    log_dir.mkdir(parents=True, exist_ok=True)
    return log_dir

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 15548f9

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 2660fac

@bong-water-water-bong
bong-water-water-bong merged commit 76c1470 into main Nov 4, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/hyperloom-per-measurement-logs-rebased branch November 4, 2026 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review effort 3/5 tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied

Projects

None yet

1 participant