#269: assembling a target held three copies of it, not one — 3.06x peak to 1.13x - #271
Merged
Merged
Conversation
Measured before changing anything, and the issue's diagnosis was a third of the story. WHAT THE MEASUREMENT SHOWED. A synthetic run through the real TargetLease.load — producer-format .tap.zip shards, real read_shard, tracemalloc and peak RSS agreeing within 2% — at 36 shards x 20,000 cells x 200 samples: peak, tracemalloc 3.06x the delivered frame -> 1.13x peak, RSS delta 3.02x -> 1.06x The 3x was three roughly equal thirds, and #269 named only the first: every shard's bytes resident together; every decoded per-shard frame held for the stack; and the np.concatenate result allocated while those were still alive. The ratio held at both 12 and 36 shards, so it is the shape and not the scale. Fixing only the part the issue described would have reached about 2x. Two false starts worth recording, because both would have produced a confident wrong number. My first harness had the fake store return pre-built objects, so the download allocation fell outside the measurement window and the peak read 2.04x. The obvious fix — return bytes(blob) — allocates nothing, because bytes(b) is b when b is already bytes; so does b"" + b. It took a bytearray to get a genuine copy. The 3.06x only appeared once the store actually allocated, which is what a real download does. THE FIX IS ONE LOOP. frames_for_target takes fetch_shard_bytes(name) instead of a filled dict, drops each shard's bytes the moment they are decoded, and writes each shard into a manifest-sized buffer instead of stacking and concatenating. Peak is now the finished frame plus about one shard; the residual 0.13x is 2/n_shards, which is why 12 shards measures 1.36x and 36 measures 1.13x. The buffer's slots are sized from expected_cell_count, which this function already enforced per shard — and the enforcement runs BEFORE anything is written, so a shard whose row count disagrees is refused rather than straddling two months' slots. NOTHING ABOUT THE PRODUCT CHANGED. The wire is byte-frozen (ADR-013) and tests/test_wire_fixture.py compares delivered artifacts against checked-in bytes. It passes unchanged, which is the whole claim: an assembly change with no output change. AT PRODUCTION SCALE, 64,742 cells x 36 months at ADR-013 §5's "~1000 samples per cell" (the one input taken from the contract rather than measured), one target's frame is 8.68 GB, so peak falls from about 26.6 GB to 9.8 GB per target. THE MANAGER'S HISTORICAL FRAME IS NOT THE ELEPHANT. #269 also notes _historical_frame is held from _read through _save. By its declared dimensions — 64,742 cells x 438 months = 28,356,996 rows — that is about 108 MB at one float32 column, 1.2% of a single forecast target frame. Recorded in C-101 rather than filed as its own issue: a separate issue implying comparable cost would misdirect whoever picked it up. GUARDED by asserting the fetch/decode interleaving rather than a byte count — a memory threshold in a test is a flake on a busy machine, while "fetch, decode, fetch, decode" is exactly the property that bounds the peak. Mutation-proven: restoring the up-front dict yields ['fetch','fetch','fetch','decode','decode','decode'] and it fails. The register guard also caught me inventing a cross-reference to a register entry C-126 that does not exist; the 126 is an issue number. Suite 450 passed / 1 skipped / 39 xfailed, ruff clean. Closes #269. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review of the assembly change: sizing the buffer from the first shard MOVED a
constraint. A run whose shards disagree on draws per cell used to fail inside
np.concatenate; it now fails inside the assignment. Both are bare numpy ValueErrors —
new: could not broadcast input array from shape (6,2) into shape (6,4)
old: all the input array dimensions except for the concatenation axis must match
exactly, but along dimension 1, the array at index 0 has size 4 ...
neither names the shard, neither says "draws", and the second is only wordier. That is
the C-99 shape exactly, caught before it could bite rather than after, and the constraint
is now mine rather than incidental, so the refusal should be too.
Mutation-proven: deleting the check brings the bare numpy error straight back.
Suite 451 passed / 1 skipped / 39 xfailed, ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… false claims Five reviewers on the assembly change. The memory result held — one reproduced it independently at 3.084x -> 1.158x against my 3.06x -> 1.13x, and separately confirmed that `del shard_bytes` really does free the bytes (np.load on a BytesIO copies; OWNDATA=True, base=None), that ordering is equivalent, and that array flags match np.concatenate exactly. What did not hold was the evidence I claimed for it. NOTHING IN THE SUITE ASSEMBLED MORE THAN ONE SHARD. I wrote that tests/test_wire_fixture.py passing unchanged proved the product had not changed. That file does not reference the assembly at all — zero mentions of frames_for_target, TargetLease or track_a_source. The real end-to-end proof is test_hop_b_sink_e2e.py, and its fixture has ONE shard, so `position * expected_cell_count` never ran above zero. The interleaving guard could not have covered it either: its three shards are byte-identical copies, so any ordering bug survives it. So the core of the rewrite shipped unverified. There is now a test that assembles three shards with distinct values, months and units and asserts equality with np.concatenate in manifest order. Mutation-proven three ways: reversing the slot index, an off-by-one in stop, and leaving the identifiers unwritten each fail it. A STORE FAULT WAS BEING BLAMED ON THE MANIFEST. frames_for_target reads a KeyError from the fetch callback as "this shard was never pinned", and my lambda put `self.store.download(...)` inside the same try. A KeyError from anywhere in the client — a response indexed with [] rather than .get(), which is exactly how C-99 happened one layer down — would have surfaced as "manifest lists shard X but its bytes were not provided", with `from None` discarding the traceback that said otherwise. Verified by mutation: the old lambda produces that false diagnosis verbatim. The lookup and the download are now separate, and a store KeyError raises a named SourceSelectionError chained to its cause. expected_cell_count SIZES AN ARRAY NOW. It used to sit on one side of a `!=`, where 6.0 compares equal to 6 and passed. It now reaches np.empty and raises a bare "TypeError: 'float' object cannot be interpreted as an integer", naming neither the field nor the manifest — from a document that crossed a repository boundary and whose read side only checks that the key is present. Guarded, with bool and non-positive covered. FOUR FALSE CLAIMS IN MY OWN PROSE. - "the residual is 2/n_shards": fits none of its own numbers. 2/12 is 0.17 against a measured 0.36. Re-measured at 12, 24 and 36 shards: the overhead is CONSTANT at about 4.5 shard-widths (~70 MB), so the ratio falls as 1/n — 1.36x, 1.19x, 1.13x. - "ADR-013 §5's ~1000 samples per cell": that line is under §0. §5 is the GAUL sidecar. - the test_wire_fixture citation, above. - a docstring quoting the numpy error as "into shape (12,4)" when the failing assignment touches a (6,4) slice. THE HISTORICAL FRAME IS FILED, NOT GLOSSED. #269 lists "the historical frame is released, or not held" as an acceptance criterion; this change does not meet it. It is now #273, carrying the measurement (108 MB, ~1.2% of a target frame) so nobody picks it up thinking it is comparable. Closing #269 with that quietly unmet was the alternative. Suite 457 passed / 1 skipped / 39 xfailed, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closed
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Measured before changing anything, and the issue's diagnosis turned out to be a third of the story.
What the measurement showed
A synthetic run through the real
TargetLease.load— producer-format.tap.zipshards, realread_shard,tracemallocand peak RSS agreeing within 2% — at 36 shards x 20,000 cells x 200 samples:The 3x was three roughly equal thirds, and #269 named only the first:
np.concatenateresult, allocated while (2) was still alive.The ratio held at both 12 and 36 shards, so it is the shape and not the scale. Fixing only the part the issue described would have reached about 2x.
Two false starts, recorded because both would have produced a confident wrong number
My first harness had the fake store return pre-built objects, so the download allocation fell outside the measurement window and peak read 2.04x. The obvious correction —
return bytes(blob)— allocates nothing, becausebytes(b) is bwhenbis already bytes; and so doesb"" + b. It took abytearrayto get a genuine copy. The 3.06x only appeared once the store actually allocated, which is what a real download does.The fix is one loop
frames_for_targettakesfetch_shard_bytes(name)instead of a filled dict, drops each shard's bytes the moment they are decoded, and writes each shard into a manifest-sized buffer instead of stacking and concatenating. Peak is now the finished frame plus about one shard — the residual 0.13x is2/n_shards, which is why 12 shards measures 1.36x and 36 measures 1.13x.The buffer's slots are sized from
expected_cell_count, a declaration this function already enforced per shard, and the enforcement runs before anything is written — so a shard whose row count disagrees is refused rather than straddling two months' slots.Nothing about the product changed
The wire is byte-frozen (ADR-013) and
tests/test_wire_fixture.pycompares delivered artifacts against checked-in bytes. It passes unchanged, which is the whole claim: an assembly change with no output change.At production scale
64,742 cells x 36 months, at ADR-013 §5's "~1000 samples per cell" — the one input taken from the contract rather than measured — one target's frame is 8.68 GB, so peak falls from about 26.6 GB to 9.8 GB per target, roughly 16.8 GB saved. Run-0's OOM kill recorded
anon-rss:23778224kB; that incident's root cause was pandas on the historical leg and is not this, but the magnitude says this leg alone would have exhausted the same box.The manager's historical frame is not the elephant
#269 also notes
_historical_frameis held from_readthrough_save. By its declared dimensions — 64,742 cells x 438 months = 28,356,996 rows — that is about 108 MB at one float32 column, 1.2% of a single forecast target frame. Recorded in C-101 rather than filed as its own issue: a separate issue implying comparable cost would misdirect whoever picked it up.Guarded
test_shards_are_fetched_one_at_a_time_not_all_up_frontasserts the fetch/decode interleaving, not a byte count — a memory threshold in a test is a flake on a busy machine, while "fetch, decode, fetch, decode" is exactly the property that bounds the peak. Mutation-proven: restoring the up-front dict yields['fetch','fetch','fetch','decode','decode','decode']and it fails.The register's own reference guard also caught me inventing a cross-reference to a register entry
C-126that does not exist — the 126 is an issue number.Verification
ruffclean; 450 passed, 1 skipped, 39 xfailed.Closes #269.