Learned low-rank activation codec for split boundaries - #1411
danielwinterw wants to merge 3 commits into
Conversation
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
26e5da6 to
75e5235
Compare
16e3671 to
9d4dfcf
Compare
75e5235 to
185433f
Compare
9d4dfcf to
cc574a1
Compare
b32cdc1 to
5fee634
Compare
3752dd6 to
d879dcb
Compare
272e6db to
fc57e1d
Compare
b122b12 to
ce2c2fd
Compare
fc57e1d to
0b286e9
Compare
i386
left a comment
There was a problem hiding this comment.
Reviewed end-to-end: protocol tagging (rank packed into the reserved field's high bits with the dtype tag in the low byte — safe since existing dtypes had zero high bits), codec encode/decode math, size arithmetic with checked multipliers, CLI/env plumbing, and the allowlist regen.
Clean aspects: strictly opt-in (Lowrank bails at startup without a codec), all existing dtype paths unchanged, decode validates rank and payload size, fit is deterministic, and there are round-trip, persistence, and degenerate-shape tests.
Two non-blocking notes for the follow-up plan:
- Codec identity is not verified across a boundary. The decode path checks that the frame's rank matches the loaded codec's rank, but two different codecs of the same rank will silently corrupt outputs (per the doc comment, both stages must hold byte-identical tensors — nothing enforces it). Suggest hashing the codec tensors at load and advertising/verifying the digest at connection setup, or at minimum in the state header handshake.
- Env-only config (
MESH_LLM_BOUNDARY_CODEC) applies one codec to all boundaries on a stage; with multi-stage splits this will collide once per-boundary codecs exist. Fine for now given the tracked plan, just flagging the seam.
Also minor: the {}x compression print in fit_boundary_codec_cli divides width*2 by rank + 4 — the per-token f32 scale makes it rank + 4/d per row, so the printed factor is pessimistic for small d.
| .state | ||
| .dtype() | ||
| .context("read activation wire dtype")? | ||
| == skippy_protocol::binary::WireActivationDType::Lowrank |
There was a problem hiding this comment.
This is the codec-identity gap from my review, flagged inline: the check validates only the rank, so two different codecs of the same rank on the two sides of a boundary will decode garbage silently — the doc comment's "both stages must hold byte-identical codec tensors" invariant is unenforced. Suggest hashing the codec tensors at load (e.g. SHA-256 over the .skbc payload) and advertising/verifying the digest at connection setup so mismatched codecs fail loudly instead of corrupting outputs.
0b286e9 to
9deb3c8
Compare
|
Thanks for the approve. On the codec-identity gap — agreed, and it is the one I would least want to ship silently: two same-rank codecs decoding garbage with no error is exactly the failure mode that looks like a model quality problem. Hashing the .skbc payload at load and verifying the digest at connection setup is the right shape, and it fits the package-hash keying already listed as a follow-up in the PR description. Not doing it in this PR: the digest wants to ride the stage handshake alongside the other capability negotiation, and this PR is deliberately inert until the embedded forward path is wired (auto never resolves to lowrank). I would rather land the negotiation and the forward path together than add a half-enforced check now. Tracking it as the blocking item before this comes off draft. |
9deb3c8 to
347bcb3
Compare
da425d1 to
a8eeaa7
Compare
347bcb3 to
34b3ba2
Compare
|
Reviewed at The framing decision is good: putting the rank in the header so frame sizes stay derivable at read time is the right call, and it's what keeps Three things, in order of how much I care. 1. A codec mismatch is silently wrong output, not an error
You flag this ("codec identity is keyed informally for now — package-hash keying is the planned follow-up"), and package-hash keying is the right end state. But the gap between "informally keyed" and "silently wrong" is the part I'd close before this leaves draft, because it's the failure that will cost someone a day. A cheap interim: hash the 2.
|
- WireActivationDType::Lowrank (tag 3): the codec rank rides in the high bits of the state header's reserved field, so frame sizes stay derivable at read time and existing dtypes are unaffected. - BoundaryCodec: PCA codec (orthogonal iteration, deterministic seed) with per-token int8 quantized coefficients; ~8x vs f16 at rank d/8. Fitted offline via the new fit-boundary-codec subcommand from activations captured with SKIPPY_CAPTURE_BOUNDARY_ACTIVATIONS. - serve-binary grows --boundary-codec; lowrank encode/decode intercepts in the stage forward and input paths; embedded stages can load a codec via MESH_LLM_BOUNDARY_CODEC (full model-config/package-manifest plumbing is tracked in the codec plan). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
34b3ba2 to
242c425
Compare
…non-finite scales - dtype() rejects non-zero reserved high bits for every dtype but Lowrank. `reserved` used to be exactly the dtype tag, so masking alone silently accepted frames this field had always rejected — a loss of validation affecting existing dtypes, not just the new one. - The codec claims stage generation 6. #1409 defines generation 5, and a shipped generation-5 peer predates this change to `reserved`, so riding on 5 would turn an excluded-at-planning-time peer into a runtime frame error. - decode rejects a non-finite per-token scale instead of propagating NaN activations into the model. - validate_lowrank names the write-path validation call that previously read as a discarded value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
242c425 to
19b189c
Compare
|
Thanks for going through the allocation path — good to have that confirmed rather than assumed. 2. dtype() lost validation. Fixed in 19b189c and you're right that this was the worst of the three: it silently weakened a field that had been fully constrained, for every dtype rather than just the new one. 3. Generation. Bumped this branch to generation 6. Reasoning matches yours: #1409 is the base and merges first, a release can be cut between the two, and at that point a shipped gen-5 peer predates the Nits: 1. Codec identity — the one I'm not doing here, and I want to be straight about why. I agree it's the failure that costs someone a day, and your interim (32-bit fingerprint over the If you'd rather see the fingerprint land now on the grounds that a silent-garbage failure outweighs the tidiness of doing negotiation once, say so and I'll do it — that's a reasonable call and I don't feel strongly enough to argue it. |
|
Closing as superseded by #1482 (commit 7369cfd). This draft encodes Lowrank and its rank through WireActivationDType and StageStateHeader.reserved. #1482 removed the dtype selector and reserved field, made raw F32 the only stage activation representation, and bumped the state and protocol contracts. The branch also records codec identity negotiation and real-model quality gates as blockers before it could leave draft. Reviving low-rank boundary compression now needs a fresh design against the current raw-F32 wire rather than merging this implementation. |
Draft: learned low-rank activation codec for split boundaries
A per-boundary PCA codec targeting ~8x vs f16 on the inter-stage wire
(4096-wide f16 = 8 KB/token -> rank-512 int8 ~ 0.5 KB, rank-1024 ~ 1 KB):
WireActivationDType::Lowrank: the rank rides in the high bits of thestate header's reserved field, so frame sizes stay derivable at read time
and existing dtypes are byte-identical.
BoundaryCodec: orthogonal-iteration PCA (deterministic seed), per-tokenint8 quantized coefficients,
.skbcfile format with dimension sanitybounds. Fit offline with the new
fit-boundary-codecsubcommand fromactivations captured via
SKIPPY_CAPTURE_BOUNDARY_ACTIVATIONS.serve-binary --boundary-codec+--activation-wire-dtype lowrankenable it on the standalone stage path; embedded stages can load a codec
via
MESH_LLM_BOUNDARY_CODEC.Draft because: protocol + tooling are complete and unit-tested
(roundtrip error bounds on synthetic low-rank data), but there are no
real-model quality gates yet (perplexity delta, suffix accept-rate delta),
no package-manifest shipping of codec tensors, and
autodtype resolutiondeliberately does not select lowrank until the embedded forward path is
wired. Codec identity is keyed informally for now — package-hash keying is
the planned follow-up.