Add a packed inference representation for trained forests - #24
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLASS-DISTRIBUTION divides by a zero sum for both NIL and a zero-length array, returning a uniform distribution without signalling either way. A split whose sampled attribute is constant over a node's rows yields threshold = min = max from MAKE-RANDOM-TEST, and with the >= convention every row goes left, leaving the right child a leaf with exactly such an empty array -- rare (3 of letter's 26936 leaves at :max-depth 20) but not hypothetical. check-packable now requires a leaf's sample-indices to be both present and non-empty, with a detail message that distinguishes the two cases. Added packed-topology-refuses-a-leaf-with-no-samples-to-count, which empties one leaf directly since the condition is too rare to provoke reliably from the synthetic fixture (0 of 5835 leaves observed there). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Layers a class-distribution payload on the packed topology: build-packed-classifier flattens each leaf's distribution into either a dense n-leaf x n-class table or a CSR (offsets/class/probability) form chosen by row size, and packed-predict sums per-tree distributions into a caller-supplied accumulator the same way class-distribution-forest does. packed-verify checks both class and full distribution element-by-element against the library, requiring exact equality rather than argmax agreement.
Adds src/packed/io.lisp: PACKED-SAVE writes a magic/version/byte-order header followed by the topology and payload arrays as little-endian words; PACKED-LOAD reconstructs a packed-classifier from one, refusing files with a bad magic, unknown version, mismatched byte order, or truncated data via PACKED-LOAD-ERROR. Byte order is recorded, not converted -- a model must be reloaded on a machine of the same order. Float bits go through SBCL/CCL fast paths with an ANSI-only portable fallback used as the cross-implementation reference.
INTEGER-DECODE-FLOAT leaves a denormal's significand unnormalised, so the biased-exponent arithmetic wrote a bogus non-zero exponent field for every denormal (LEAST-POSITIVE-SINGLE-FLOAT encoded as #x00800001 where IEEE-754 says #x00000001). Branch on whether the significand is already normalised and shift denormals directly into the fraction field instead. packed-float-bits-round-trip gains true denormals and both signed zeros, and now cross-checks the portable decoder against the fast encoder in both directions -- the decode direction, and denormals specifically, were never exercised before, which is why the bug survived the original test.
…l dependencies src/utils.lisp and src/random-forest.lisp reference these packages qualified throughout but never named them in their DEFPACKAGE forms. Package-inferred- system derives a file's dependencies only from its DEFPACKAGE clauses, so nothing ever told ASDF to load them on a path that does not go through the top-level cl-random-forest system's explicit :depends-on -- cold-loading cl-random-forest/src/packed on its own failed with "Package LPARALLEL does not exist", then, once that was fixed, the same for SVMFORMAT and CLOL.VECTOR in turn. SVMFORMAT and CLOL/CLOL.VECTOR are nicknames whose systems (cl-libsvm-format, cl-online-learning) don't match the downcased package name ASDF would otherwise guess, so cl-random-forest.asd now registers both mappings with ASDF:REGISTER-SYSTEM-PACKAGES. This is a pre-existing defect, not something the packed system introduced -- packed is simply the first thing that ever tried to load a piece of this project without going through the top-level system. Verified on a genuinely cold fasl cache, and confirmed ASDF's own computed dependency graph (ASDF:COMPONENT-SIDEWAY-DEPENDENCIES) now names all of these explicitly rather than relying on load-order accidents. No new compiler warnings versus master; full test suite still passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLAUDE.md and README.org now cover cl-random-forest/src/packed: how to load it, what it buys over predict-forest, and the two things to know before relying on it (it's a snapshot of the forest at build time, and it refuses forests pruned with :remove-sample-indices? t). Give src/packed.lisp the :clrf.packed nickname the README's examples use. cl-random-forest-test/packed was already wired into the aggregate system by Task 1; this just documents the eighth suite alongside the other seven. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
packed-load reconstructed LEFT, RIGHT, ROOTS, CSR OFFSETS and CSR CLASS straight from file bytes with no range checks, unlike build-packed-topology, which validates exhaustively before its own (safety 0) traversal ever runs. A one-byte header corruption (n-class decremented from 4 to 2) loaded with no error into a model whose dense table was then read at the wrong row stride, every leaf distribution silently wrong. Worse, a corrupt CSR class[i] >= n-class was never caught, which at (safety 0) is an out-of-bounds *write* into the caller's accumulator in packed-predict's CSR path, not just a wrong answer. Add validate-topology-arrays and validate-csr-arrays, run before packed-load returns. They check that every LEFT/RIGHT/ROOTS entry addresses a real internal node or leaf, TREE-LEAF-OFFSETS is a non-decreasing partition starting at 0, the header's four counts are sane (n-tree and n-class positive, all four non-negative), and, for a CSR file, OFFSETS is a non-decreasing partition ending at (length CLASS) = (length PROBABILITY) with every class id in range. Thresholds and leaf probabilities are deliberately left unchecked -- any bit pattern is a legal float, and NaN is not this layer's problem. Also, while touching packed-load: bind read-array's results as let* variables in file order before constructing the topology, instead of passing each read-array call inline as a keyword argument. :left and :right share type and length, so transposing those two keywords was a cosmetic-looking edit that would have silently swapped every split's branches with no error at any layer. And fix packed-save's docstring, which claimed the writing machine's byte order is recorded and a mismatch refused -- write-u32/read-u32 and single-float-to-bits are endianness-independent by construction, so the format never needed that, and +byte-order-probe+ is just a second, distinctive magic number. t/packed.lisp: the truncated-file corruption test reopened the file with :if-exists :supersede and wrote zero bytes over the first half, which zeroed the magic along with everything else and never reached the short-read paths it exists to exercise -- the first case in the same test already covers a corrupted magic. Fixed to read the original bytes into a buffer and truncate that instead. Added two more corruption cases alongside it: a decremented n-class (built CSR, so a class id in the file overflows the corrupted count) and a corrupted LEFT entry, whose byte offset is computed from the header rather than hardcoded. Both loaded with no error before this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src/packed/classifier.lisp: document the two predict paths' differing accumulator state. packed-predict normalises ACC in place -- on return it holds the class distribution, which packed-verify relies on to read straight out of it -- while packed-predict-batch leaves ACCS holding raw per-tree sums, dividing by n-tree only in a local while picking each row's argmax, and never writing that quotient back. Also state packed-predict-batch's shape contract: ACCS needs at least (- end start) rows and OUT at least (- end start) elements, since nothing cross-checks make-packed-accumulators' independent tile argument against the (start, end) actually predicted, and at (safety 0) either being too small is a silent out-of-bounds write. CLAUDE.md: build-packed-topology rejects a forest that has been *pruned*, not one merely built with :remove-sample-indices? t as previously stated -- pruning!'s delete-children! turns an already-split node, whose sample indices were nil'd when it was split, back into a leaf without restoring them (issue #14). A default-built, unpruned forest packs fine regardless of what :remove-sample-indices? was given at construction, which t/packed.lisp already asserts directly; the old wording just had the wrong trigger. Also drop the citation of docs/superpowers/specs/2026-08-06-packed-forest-design.md, which .gitignore excludes from every clone. README.org: the packed-inference example built on *forest*, which by that point in the file has been pruned with its default :remove-sample-indices? t construction -- exactly the combination build-packed-classifier refuses. Add a sentence so a reader following the file top to bottom knows to rebuild with :remove-sample-indices? nil before packing a forest they intend to prune, instead of hitting an unexplained packed-build-error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3131c3adf4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (dotimes (i n-internal) | ||
| (check-child-reference "left" i (aref left i) n-internal n-leaf) | ||
| (check-child-reference "right" i (aref right i) n-internal n-leaf)) | ||
| (dotimes (tree n-tree) | ||
| (check-child-reference "roots" tree (aref roots tree) n-internal n-leaf)) |
There was a problem hiding this comment.
When a packed file contains an in-range self-reference or a longer cycle in left/right, these checks accept it because they validate only bounds. If such a node is reachable from a root, packed-leaf loops forever under (safety 0), so one malformed model can hang every inference request that follows that path; validate that all root-reachable paths terminate at leaves before returning the classifier.
Useful? React with 👍 / 👎.
| (let* ((feature (read-array s n-internal 32 '(unsigned-byte 32) #'identity)) | ||
| (threshold (read-array s n-internal 32 'single-float #'bits-to-single-float)) | ||
| (left (read-array s n-internal 32 '(signed-byte 32) #'s32-of-u32)) | ||
| (right (read-array s n-internal 32 '(signed-byte 32) #'s32-of-u32)) | ||
| (roots (read-array s n-tree 32 '(signed-byte 32) #'s32-of-u32)) | ||
| (tree-leaf-offsets (read-array s n-tree 32 '(unsigned-byte 32) #'identity))) | ||
| (validate-topology-arrays n-internal n-leaf n-tree left right roots | ||
| tree-leaf-offsets) |
There was a problem hiding this comment.
A packed file can put any 32-bit value in feature, and this array is never checked before packed-leaf uses the value as a datamatrix column index with bounds checks disabled. Thus a corrupted model that otherwise passes packed-load can cause an out-of-bounds memory access during ordinary prediction; serialize the training dimension and reject feature indices outside it, or retain the required dimension and validate the inference matrix before unchecked traversal.
Useful? React with 👍 / 👎.
| (let* ((bytes-per (floor bits 8)) | ||
| (buffer (make-array (* n bytes-per) :element-type '(unsigned-byte 8))) | ||
| (got (read-sequence buffer stream)) | ||
| (out (make-array n :element-type element-type))) |
There was a problem hiding this comment.
When a malformed or truncated model advertises a very large n-internal, n-leaf, n-class, or CSR nnz, read-array allocates buffers based entirely on that untrusted count before checking how many bytes remain. For example, a 36-byte file with n-internal = #xffffffff attempts a multi-gigabyte allocation instead of signaling packed-load-error, potentially exhausting the process; compare each requested byte count with the remaining file length and implementation array limits before allocation.
Useful? React with 👍 / 👎.
Codex's review of #24 found three gaps in what packed-load validates, all confirmed against the code. Everything downstream runs at (safety 0), so a value that gets past the loader is not caught anywhere else. A cycle in left/right passed validation. Bounds alone do not make the walk terminate: packed-leaf loops while the node index is non-negative, so left[i] = i is in range and hangs prediction with no recovery. The fix is one comparison, because build-packed-topology already numbers a subtree after its root -- emit takes its slot before recursing. Requiring each step to move forward through a bounded array makes termination structural rather than circumstantial, and check-forward-numbering now asserts the invariant at build time so a later change to the numbering fails there with a clear message instead of turning into files the loader mysteriously rejects. feature was never checked at all. check-packable bounds it by the forest's datum-dim when building, but the topology did not keep that width, so it could not be rechecked and packed-leaf used the value as a datamatrix column subscript at (safety 0) -- a read at an arbitrary offset. datum-dim is now a topology slot and a header field. Since it is untrusted in a file, it is half a pair: the loader bounds feature by the claimed width, and check-datamatrix-width bounds the claimed width by the matrix actually passed to the walk, giving feature < datum-dim <= the real column count. That second check also catches the ordinary form of the same mistake, with no file involved: predicting with a narrower dataset than the forest was trained on, until now a silent read past the end of a row. It costs nothing measurable -- 39-41k predictions/s on a 500-tree depth-10 forest with and without it, a spread smaller than the run-to-run noise. read-array sized both its buffers from a header count before it could discover the file was shorter, so a 36-byte file declaring 4294967295 internal nodes asked for 16 GB and died of heap exhaustion rather than signalling. Each array's declared length is now compared with the bytes actually remaining first. Header version 2: datum-dim is appended as an eighth u32 field. Nothing has been released reading version 1. Verified each rejection fires for its own reason rather than incidentally, by corrupting a saved model one field at a time and reading the messages back: the cycle trips the forward-motion rule, the feature index trips the width bound, and the inflated count is refused for wanting 17179869180 bytes with 57296 left in the file. The untouched file still loads and still agrees with the in-memory classifier on all 1500 predictions.
|
All three findings confirmed against the code and fixed in 934f556. Two of them Cyclic node references. Bounds alone do not make the walk terminate: The feature dimension. One refinement on the suggestion: Unbounded allocation. Verified each rejection fires for its own reason rather than incidentally, by corrupting a The untouched file still loads and still agrees with the in-memory classifier on all 1500 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 934f55644a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (unless (and indices (plusp (length indices))) | ||
| (error 'packed-build-error |
There was a problem hiding this comment.
When training selects a feature that is constant within a node, the existing splitter can legitimately create a zero-length child leaf; the adjacent comment notes this occurs on the letter dataset at :max-depth 20. Unlike a NIL sample array stranded by pruning, that empty array accurately represents zero samples, and class-distribution deterministically returns the uniform distribution that packed inference could copy exactly. Requiring plusp therefore prevents otherwise usable, unpruned forests from being packed, contrary to the documented behavior; reject missing indices but retain the baseline distribution for present empty arrays.
Useful? React with 👍 / 👎.
…tion ccl-bin CI failed on four denormals -- 1.4012985e-45, its negative, 1.729903e-41 and 5.877472e-39 -- with the portable encoder disagreeing with CCL's native one. The round trip and the portable decoder both passed on the same values, which localised it: CCL's bits are the correct IEEE pattern and the portable encoder was producing something else. INTEGER-DECODE-FLOAT owes its caller only a (significand, exponent) pair whose product is the magnitude. Which member of that equivalence class it returns is up to the implementation, and for denormals the two we run on disagree: SBCL leaves the significand unnormalised, so LEAST-POSITIVE-SINGLE-FLOAT comes back as 1 x 2^-149, while CCL normalises it to 2^23 x 2^-172. The encoder decided "is this a denormal?" by testing the significand against 2^23, which reads the host's convention rather than the format. On CCL that test is false for every denormal, so it took the normal-number branch and wrote a wrapped exponent field: 5.877472e-39 encoded as 00000000, a denormal silently becoming zero. Normalising the pair first and letting the biased exponent decide is true of either convention. Reduction now runs in both directions, since the pair is pinned down only up to doubling the significand and dropping the exponent. The arithmetic moved into %FLOAT-PARTS-TO-BITS, taking the parts rather than a float, so both conventions can be fed in from any host. That is what the old test could not do: it compared the portable encoder against the native one, so on SBCL the host never produced the pair that breaks it and only CCL could see the bug. The new test supplies the pairs directly and fails on SBCL too -- verified by restoring the pre-fix branch, which fails 13 assertions here, the first four being exactly the values CI reported. Swept 53776604 bit patterns (every denormal, every exponent, and every pattern in the two smallest normal binades) through four representations of each: as decoded, normalised, reduced to an odd significand, and over-scaled. Zero disagreements.
The matrix is three jobs, not four: the workflow excludes ccl-bin on macOS because those binaries are no longer distributed. Roswell has no ccl-bin for ARM64 Linux either, so on an aarch64 machine the CCL half cannot be run locally at all -- which is how a denormal encoding bug reached CI in this branch. Says so, with the two CCL differences that have actually bitten.
|
ccl-bin CI was red on four denormals — Not a packed-inference bug: the portable float encoder was reading the host's decode
Normalising the pair first and letting the biased exponent decide holds under either The failure pattern is what localised it. For each bad value the round trip passed and The test also needed fixing, not just the code. It compared the portable encoder against the Swept 53776604 bit patterns (every denormal, every exponent, and every pattern in the two Worth noting for anyone reproducing this: Roswell has no |
1 participant
Adds
src/packed/, a separate ASDF system that flattens a trained classification forestinto specialised simple arrays for inference. It is a derived read-only view: training,
pruning, feature importance and reconstruction all keep using the
nodestructs.The design and the measurements behind it are in
docs/packed-forest-layout.md.Why
predict-forestwalks linkednodestructs and, at every leaf on every prediction,recounts that leaf's class histogram from its
sample-indices— leaf values aredeliberately not cached. It is also not reentrant: it accumulates into
forest-class-count-arrayand each leaf read overwritesdtree-class-count-array, bothslots on the shared model, so predicting from several threads returns wrong answers
silently.
What it gives
On 500-tree depth-10 forests, against
predict-forest, measured on ARM64 SBCL 2.6.7:Predictions are bit-identical, not merely argmax-identical.
packed-verifycomparesdistributions element by element against
class-distribution-forest; checking the argmaxalone is too weak, since two different distributions can share one. An earlier sketch in
this repository substituted a majority vote for the distribution sum and differed on 48 of
10000 MNIST predictions with nobody noticing.
Layout
Internal nodes only, one array per field. A leaf is a negative child index,
~leaf, so thetraversal's continuation test is the leaf test and a leaf costs no extra read.
Leaf numbering is derived rather than incidental: leaf number is
tree-leaf-offsets[tree] + the tree's own leaf index, which is by construction the indexGlobal Refinement uses. During prototyping the two happened to coincide because both walks
recurse left then right; deriving it makes the coincidence a guarantee, so the node ordering
can change later without silently renumbering the refine feature space.
Two leaf payloads.
:autochooses CSR above 16 classes, dense at or below — the rule is"does a dense row exceed a cache line",
4 * n-class > 64, not the compression ratio, whichis the intuitive choice and picks wrong: MNIST compresses 2.4x and gets slower (0.82x),
because with ten classes a dense row is 40 bytes and CSR's two offset loads plus a class
index per non-zero cost more than the bytes saved. Skipping zeros is exact rather than
approximate — every value is non-negative, so omitting a
+ 0.0changes no sum.Serialisation
packed-save/packed-load. All slots are specialised simple arrays, so the format is aheader followed by the arrays — no parsing, no reconstruction. It is byte-order independent
by construction:
write-u32is explicitly LSB-first andsingle-float-to-bitsyields anIEEE-754 bit pattern.
packed-loadvalidates the arrays before returning, because everything downstream runs at(safety 0): child and root references in range or decoding to a valid leaf, leaf offsetsnon-decreasing, header counts sane, CSR offsets monotonic and every class index below
n-class. Without this a corrupt CSR class index is an out-of-bounds write into thecaller's accumulator, and a one-byte header edit loads silently into a wrong model.
Two things to know
A packed model is a snapshot. Prune the forest afterwards and the packed copy goes on
predicting with the old structure, silently. That hazard exists in memory too, so it is
documented rather than solved only for files.
build-packed-topologyrefuses a forest that has been pruned:pruning!'sdelete-children!turns an already-split node — whose sample indices were nil'd when it wassplit — back into a leaf without restoring them, so that leaf's class distribution would
come out uniform rather than signalling (issue #14). A default-built, unpruned forest packs
fine regardless of
:remove-sample-indices?;t/packed.lispasserts this directly. To bothprune and pack, build with
:remove-sample-indices? nil.Also in this branch
src/utils.lispandsrc/random-forest.lispreferencedlparallel,alexandria,svmformat,clolandclol.vectorwithout declaring them, relying on the top-level.asdhaving loaded everything. No subsystem in this project has ever been independentlyloadable; packed is simply the first thing that tried, and it failed on a cold fasl cache.
Now declared, with
asdf:register-system-packagesfor the three nickname packages.Testing
New
cl-random-forest-test/packedfeature system, 11 tests, in the aggregate and so in CI.It uses the fixture's synthetic data and needs no network. Full suite green: 8 systems.
Not in scope: regression forests (the leaf value is a scalar, a small change, but not one
measurement exists for it yet), and anything that changes training.
🤖 Generated with Claude Code