Repository navigation
tools: kernel-set-check.sh — is a shipped set self-consistent, and in one form? (refs #2106) - #2120
bong-water-water-bong wants to merge 4 commits into
Conversation
…consistent and in one form (engine#2106)
PR Reviewer Guide 🔍(Review updated until commit a319324)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 at 9af81f0: CHANGES
Infrastructure, not the diff — and this one is now worth a fleet-level look rather than a rerun, because the retry that exists for it did not save this run:
::error::apt-get update+install failed 4 times -- if the 404s name .debs the index lists, i…
The workflow already anticipates this precisely. ci.yml documents engine#1814: on 2026-11-11 perl-base/perl-modules-5.40/libperl5.40 404'd on .debs the fresh index had just listed (mirrors 91.189.92.22 and 91.189.91.82), turning main red for every PR; a rerun passed with no code change. The retry loop was written for that, and it re-runs apt-get update per attempt specifically so a later attempt can reach a different mirror.
So it is not "nobody thought of this" — it is that the mirror failed harder than four attempts could cover, twice within the hour (#2113 and this PR, both the same message). That is a signal about the mirror, not about either PR:
- if the instance is fixed, ↑ attempts or a pinned alternate mirror is a one-line change;
- if it is not, the durable answer is caching the .debs in the job (or a prebuilt image), because a release-blocker PR cannot wait on Ubuntu's mirror rotation.
Your PR is unaffected: kernel-set-check.sh ("is a shipped set self-consistent") is exactly the kind of pre-release check the blocker list wants, and its diff cannot influence a perl-base 404. Rerun; if it goes green this verdict is void.
That is now four infrastructure reds today (#2027 dist.tar.gz from b1, #2057 CodeQL config, #2113, this), two of them this same apt failure — the ambiguity #2093's pr_check_verdict.py exists to classify.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 9af81f0: CHANGES
Reviewed head 9af81f0 (refs #2106). This is the second verdict on this head. I found two problems, each tested on this head, and both are small.
1. The red build is this diff, not the apt mirror, so a rerun will not fix it. CI run 42957632529 has one attempt, and its failed step is "Copyright notice on every file". The apt retry step did not fail in this run. Locally at this head, tools/copyright.py --check gives missing copyright notice: tests/kernel_set_check_test.sh, rc 1. The test carries only the two-line Copyright/SPDX header, while scripts/kernel-set-check.sh carries the full Apache notice and passes.
Fix: replace the test's lines 2–3 with lines 2–15 of scripts/kernel-set-check.sh (the full notice). Tested that way: copyright.py --check rc 0, and kernel_set_check: all cases ok. The diff is +12 lines in the test.
2. A consistent plain-ELF set fails as MIXED, because the listed layout.json is counted as a sealed kernel. Every shipped manifest I checked (the 10-31c, 11-05 c25 and 11-07 c32 sets for 1024x3072x16x8) lists 10 kernel files plus layout.json. The form loop classifies every listed entry by its first four bytes, and JSON does not start with \x7fELF. So on the shipped sealed set, "11 file(s) in sealed/opaque form" is 10 kernels plus the layout file.
That happens to pass. But I built a fixture with that exact manifest shape (same listed paths, fake bytes, correct sizes and sha256s):
== kernels ELF-headed
OK 1024x3072x16x8: 10 file(s) in plain ELF form
OK 1024x3072x16x8: 1 file(s) in sealed/opaque form
FAIL 1024x3072x16x8: MIXED FORMS: 10 elf, 1 sealed -- ... the 'differ in size' refusal rc=1
== kernels sealed
OK 1024x3072x16x8: 11 file(s) in sealed/opaque form rc=0
So on the plain-ELF form, the other side of the very change #2106 is about, every correct set would be reported as the failure this check exists to pre-empt. The test fixtures do not list a layout.json, so the test cannot see it.
Fix: classify form only for kernel entries, using the same .elf/.bin filter the unlisted-file walk already uses (layout.json is already skipped there). Add a fixture arm with a listed layout.json in both forms: "consistent ELF set with layout.json: rc 0, 10 ELF".
Checked and fine
tests/kernel_set_check_test.sh: all 20 arms ok, rc 0.git merge-treeonto main: no conflicts.- The three shipped sets above all report rc 0, one form, correct name.
- Multi-set exit status:
bad, no-manifestandno-manifest, badboth give rc 2. The order does not matter, because a bad set never overwritesrc(only the no-manifest path returns non-zero fromcheck_set).
|
VERDICT: READY at Two asks, both about not becoming a second implementation of the rule:
And the remaining question for the row it serves: |
…h is visible (engine#2106)
|
Pushed The measurement that motivated it (posted on #2106): a current-main build refused the shipped set with while the set carries and a set that carries both schemes is a failure ( Deliberately a classification and not a prediction. The forward derives Also merged |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 496d66f: CHANGES
Re-review at head 496d66f (refs #2106, release-blocker). The new commit adds the naming line. Neither point of my CHANGES at 9af81f0 is addressed, and the branch now conflicts with main.
1. It no longer merges. git merge-tree --write-tree origin/main 496d66f gives CONFLICT (content): Merge conflict in CMakeLists.txt, and GitHub shows mergeable_state: dirty. CI's ci workflow has not run at this head; only CodeQL ran.
2. Copyright: still red, same file. tools/copyright.py --check at this head gives missing copyright notice: tests/kernel_set_check_test.sh, rc 1. The fix tested at 9af81f0 still applies: replace the test's two-line notice with lines 2–15 of scripts/kernel-set-check.sh.
3. A correct plain-ELF set still fails as MIXED FORMS. The form loop (scripts/kernel-set-check.sh:96) still classifies every listed entry by its first four bytes, so the layout.json every shipped manifest lists counts as "sealed". Rerun at this head on a fixture with the c32 set's exact manifest shape (same 11 listed paths, fake bytes, correct sizes and sha256s, kernels ELF-headed):
OK 1024x3072x16x8: 10 file(s) in plain ELF form
OK 1024x3072x16x8: 1 file(s) in sealed/opaque form
FAIL 1024x3072x16x8: MIXED FORMS: 10 elf, 1 sealed -- … the 'differ in size' refusal rc=1
Fix as before: classify form only for .elf/.bin entries, the filter the unlisted-file walk already uses. Add a fixture arm that lists a layout.json.
4. New in this commit: the naming line misreports the shipped set. layer_indexed matches layer\d+_\d+/, but half of every shipped set's directories carry a suffix: layer1_65/ and layer1_65np/, layer1_2081/ and layer1_2081np/, and so on. On the real c32 set the line reads:
OK 1024x3072x16x8: names: 5 layer-indexed layerN_M/…, 6 other
In fact it is 10 layer-indexed kernels plus layout.json. Today that only misreports, because MIXED NAMING needs role_named too. But this is the line meant to make a binary/set naming mismatch visible, and on the set it describes it is wrong by half. layer\d+_\d+[a-z]*/ takes the np twins, and the count should exclude non-kernel files (layout.json) the same way as in point 3. A fixture arm with a layerN_Mnp/ directory would pin it.
Still fine at this head: tests/kernel_set_check_test.sh passes ("kernel_set_check: all cases ok", rc 0), and the shipped c32 set itself reports rc 0, one form, correctly named.
|
Persistent review updated to latest commit dd23a94 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at dd23a94: CHANGES
Re-review at head dd23a94 (refs #2106). This head merges main ("keep both new test registrations"), so the CMakeLists.txt conflict from my 496d66f review is resolved: git merge-tree onto main is clean again.
The PR's own change is unchanged. The sorted +/- lines of scripts/ and tests/kernel_set_check_test.sh against the merge base are identical at 496d66f and here, 310 each. So the three open points stand as written there:
- CI is red on it.
build's failed step is "Copyright notice on every file":missing copyright notice: tests/kernel_set_check_test.sh. Fix: lines 2–15 ofscripts/kernel-set-check.shin place of the test's two-line notice. - A correct plain-ELF set fails as MIXED FORMS, because the listed
layout.jsonis classified as a kernel (scripts/kernel-set-check.sh:96). Classify only.elf/.binentries. - The naming line misses the
layerN_Mnp/directories (layer\d+_\d+/), so the shipped c32 set reads "5 layer-indexed, 6 other".layer\d+_\d+[a-z]*/fixes it.
The fixtures and probes for each are in my 9af81f0 and 496d66f reviews.
|
VERDICT: READY at Why those three, and why in that order
The positive run is the part that makes it an instrument
Two things to keep, and one to fix
Nothing else. "Not in this PR, and why" — the fix is the pin decision, elsewhere — is the right scope statement, and the check is the half a lane could land. |
…naming is visible (engine#2128)
|
Pushed #2128's item 1 is "two more sealed ELFs + four keys naming them", and its own note says what happens when the naming is wrong: a file that is missing or does not load is logged and the serve stays on the 16-row set. Correct at run time, invisible until somebody reads a log — so a pack edit whose name is misspelt is a no-op that reads like a win, and the acceptance line ( It reads both spellings the issue lists — Six new arms: present (named in One bug in my own tooling fixed on the way: the script had no option parser at all, so |
|
Persistent review updated to latest commit a319324 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at a319324: CHANGES
Re-review at head a319324 (refs #2106). The new commit adds --pack (wide/narrow set naming, engine#2128). It does not touch the three points from my 9af81f0, 496d66f and dd23a94 reviews, and all three are still true at this head:
- CI is red on this PR's own file.
build's failed step is "Copyright notice on every file", andtools/copyright.py --checksaysmissing copyright notice: tests/kernel_set_check_test.sh. Fix: lines 2–15 ofscripts/kernel-set-check.shin place of the test's two-line notice. - The form loop still classifies every listed entry (
scripts/kernel-set-check.sh:108), so thelayout.jsonevery shipped manifest lists makes a correct plain-ELF set fail as MIXED FORMS. Fix: classify only.elf/.binentries. - The naming pattern still misses the
…np/directories (:130,layer\d+_\d+/), so the shipped c32 set reads "5 layer-indexed, 6 other". Fix:layer\d+_\d+[a-z]*/.
git merge-tree onto main is clean. The fixtures and probes for each point are in the earlier reviews. Since #2106 is closed and this PR has now gone four heads with the same three items, it may be quickest to fix those three before adding further options.
|
The three points of the CHANGES at |
|
Closed in favour of #2200, which is
Keeping mine would have meant maintaining a copy that is already behind a better one, and the lane is at the PR cap anyway. Everything in this branch is in #2200. |
…ries #2120) (#2200) * tools: kernel-set-check.sh, so a shipped set says whether it is self-consistent and in one form (engine#2106) * kernel-set-check: name the naming scheme too, so a binary/set mismatch is visible (engine#2106) * kernel-set-check: --pack reads the wide/narrow set names, so a no-op naming is visible (engine#2128) * kernel-set-check: only kernels have a form, suffixed layer dirs are layer-indexed, full notice on the test Three points open on #2120 across four heads: the test carried a two-line notice (CI's copyright step failed), a listed layout.json was classified as a sealed kernel so a correct plain-ELF set failed as MIXED FORMS, and layerN_Mnp/ directories were counted as other. Five new arms; three fail on the script before this change. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> --------- Co-authored-by: bong-water-water-bong <pi@1bit.local> Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
Refs #2106 (release-blocker), support lane. This is the check the private-repo note filed on the box says "is not written" — the one that would have diagnosed the "differ in size" refusal from the set alone, with no device and no private-repo archaeology.
What #2106 actually was: the release candidate serves small models at 87-101 tok/s with the set beside it; a build of current main refused that same set with
layer ELFs: the NB 2 and 3 builds differ in size. Neither side was wrong about the kernels — binary and set came from different revisions of the private repository, one on each side of its protection change, so the loader read sealed files as plain ELFs and compared their lengths. The symptom, though, is entirely checkable from the set.scripts/kernel-set-check.sh <set dir>…answers three questions a release run should ask before spending a device row:sizeandsha256, nothing listed that is absent, and nolayer1.elfon disk that the manifest forgot (a loader may still pick that one up);\x7fELFor it does not; a mixed set is exactly the state in which one side reads the other side's files and sees "differ in size";That is the shipping RC set: self-consistent, one form, correctly named — which is the positive evidence the issue's item 1 was missing, obtained without private data.
tests/kernel_set_check_test.sh— 20 arms over fixture sets, registered askernel_set_check(thetests/*.shratchet counts it: 81 scripts, all referenced): consistent ELF, consistent sealed, mixed forms (asserting the message quotes the refusal it pre-empts), size disagreement, sha disagreement, listed-but-absent, on-disk-but-unlisted, wrong geometry name, no-manifest (rc 2, deliberately different from a bad set'src 1), and a multi-set call where the worst one decides while the good one is still reported.Not in this PR, and why: the fix for #2106 is the pin decision (
~/briefs/CL-NPURT-NOTE-2106.md, cl-npuperf's row) — this only makes the state of a set legible before anyone spends a ticket on it. Nothing here reads or ships kernel internals: it hashes files the release already ships beside the binary and reports form and consistency.