corrected PACT to be exchange not pipeline - #13
Merged
Conversation
frankmcsherry
added a commit
that referenced
this pull request
Aug 30, 2026
* corgi backend: exchange by key hash, lifting the single-worker guard The corgi backend's `arrange` used `Pipeline` and asserted `peers() == 1`, because a multi-worker run would have MIS-PLACED keys -- silently wrong, not slow. This replaces that assert with the exchange it was waiting for. `CorgiPact` / `CorgiDistributor` (corgi/exchange.rs) partition a container the way a columnar engine does, not the way timely's stock distributor does. There are no items to drain from a CorgiContainer -- it is four columns -- so taking it apart row-wise would undo the representation before the data left the worker. Instead: hash the key column once (corgi::hash, the same structural hash `present_key` prepends as a key's identifier lane), counting-sort row indices by destination, then `gather` each destination's rows into fresh contiguous columns. Contiguous is what the wire format wants; the sort is stable, so rows keep their order within a destination. A container whose rows all share a destination moves whole, with no gather at all. corgi/bytes.rs is the wire format: four length words, then the key and value columns through corgi's new byte codec (one memcpy per leaf column, no per-row framing) and the time and diff columns through columnar's Stash -- the encoder `T: Columnar` was already chosen for. Every section is a whole number of words, so a receiver can install the bytes rather than relocate them. The row backend reaches the wire through bincode, walking every Value of every row; that contrast is the point. Substituting the pact in `arrange` is the whole of multi-worker support: every other operator (linear, join, reduce, as_collection) is key-local once arrangements are placed correctly, and both sides of a join agree on placement because they route by the same function of the key. Testing: the backend gate now runs each of its 14 programs at 1, 2, 3 and 4 workers (3 to take the modulus path rather than the mask) and again over ProcessBinary channels, which serializes every exchanged container -- the multi-process path without the processes. `evaluate_with_workers` / `evaluate_with_config` are the entry points that makes that possible. Unit tests cover the partition (it is a partition; a key goes to one destination; routing does not depend on the batch, which is what makes a join correct; strided keys still spread) and the codec (round trips per shape family, truncation, and that a key column costs its payload). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * corgi exchange: reuse the placement cursor across batches The counting sort allocated its per-destination write cursor on every call, which is exactly the flat per-batch tax the other two index buffers are fields to avoid. The one per-batch allocation left is the hash column, which `corgi::hash` returns owned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * corgi container: check the four columns agree on the row count A decoded container is four independently encoded columns, and nothing below this layer can say how many rows the message is supposed to have. The time column can: it is stored per row, so its length is the count the sender actually paid for, and keys, vals and diffs all have to match it. This is the DDIR half of the hardening in corgi's #13. That codec now guarantees a structurally sound Value, but not a small one -- the payload-free constructors declare rows without spending bytes, so a Unit names a trillion rows in sixteen bytes and nothing inside corgi can call that wrong. Here it is wrong, and cheap to say so. What the check does not reach is a claim nested under a List, where flattening legitimately multiplies and the row count stops bounding the element count. That is the sender's honesty, which is the boundary a cluster-internal exchange accepts anyway; `corgi::bytes::declared_rows` is there for a caller that wants a hard ceiling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Pin corgi at the byte codec corgi #13 landed, so the rev moves from the unmerged `ddir-rem` branch to master. That is the last thing standing between this branch and a clean checkout: `corgi::bytes` is what `CorgiContainer`'s wire format is written in terms of, and until now it existed only behind a local `[patch]`. The rev also carries #11, #12, #14 and #15, since the old pin was a side branch off a master four commits older. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * corgi backend: stop applying shape-directed ops to an emptied batch A container with no rows has no shape either. Every path that rebuilds one from rows infers its columns by scanning them, so `from_updates` on an empty vector returns the shape-erased default -- `Unit` keys and `Unit` vals. Hand that to the next op and `compile_projection` lowers `$1[0]` against a `Unit`, because a `Field` is legal in the abstract; `eval_graph` then meets a column that is not a product and panics with "Field: expected a product". Reaching it takes two steps, which is why it survived this long. First an op that empties a batch while KEEPING its shape -- the columnar filter path does that, since `gather` of an empty index list is still a typed zero-row column. Then an op whose term the lowering DECLINES, taking the row-wise path and rebuilding through `from_updates`. Only after those does a third op meet the erasure. Latent since the backend was written: one worker rarely empties a batch it was given. The key-hash exchange makes it routine, and it is a chain emptying ITSELF that does the damage -- a filter drops every row and hands the erased container to the very next op in the same `ops` slice -- so the guard belongs at the top of the loop, not before it. Skipping is the whole answer, not a shortcut: every `LinearOp` maps zero rows to zero rows, so the only thing the ops could contribute is the output shape, and the input's shape is already gone. No consumer reads it -- `CorgiChunker:: push_into` drops empty containers before `concat_blocks`, which is the one place shapes must agree, and `into_updates` and the distributor both short-circuit on zero rows. Found by running the AoC 2023 suite (33 parts, known answers) through the corgi backend at several worker counts: day03 part2 answered correctly at one worker and panicked at four. With the fix all 33 give identical results at 1, 3 and 4 workers; the 4 that still disagree are the pre-existing ones that suite's README already records, and they do not vary with worker count. tests/programs/empty_batch.ddp reproduces it in the gate, built from the failing day03 export rather than guessed at -- two earlier attempts passed with the fix reverted, because the filters I reached for compiled columnar and kept their shape. Without the guard this one wedges the gate rather than failing it, since a panicking timely worker leaves the others waiting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Oops.