Change: carry the wire Tensor as the remote-L3 per-argument record - #1756
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRemote L3 protocol version 3 replaces metadata-only task tensors with full ChangesRemote tensor wire transport
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RemoteEndpoint
participant RemoteWire
participant RemoteL3Session
participant sidecar_registry
participant inner_worker.run
RemoteEndpoint->>RemoteWire: encode args.tensors
RemoteWire->>RemoteWire: validate REMOTE_SIDECAR descriptor
RemoteWire->>RemoteL3Session: decode full Tensor
RemoteL3Session->>sidecar_registry: resolve backing identity
sidecar_registry-->>RemoteL3Session: return registered backing
RemoteL3Session->>inner_worker.run: pass reconstructed Tensor view
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks 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 |
614f2cc to
28fd56d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/common/hierarchical/remote_wire.cpp (1)
459-501: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire a sidecar for every remote TASK tensor.
The endpoint can emit an absent sidecar for a non-DEVICE placeholder. The wire codec accepts that record.
_materialize_task_argsrejects it, so the task fails only after transport and decode.
src/common/hierarchical/remote_wire.cpp#L459-L501: reject absent sidecars during encoding and decoding.src/common/hierarchical/remote_endpoint.cpp#L739-L758: requiretensor_sidecar.presentfor every remote TASK tensor before building the payload.Based on the supplied protocol design: every remote TASK tensor requires an authoritative sidecar descriptor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/hierarchical/remote_wire.cpp` around lines 459 - 501, Require an authoritative sidecar for every remote TASK tensor: in src/common/hierarchical/remote_wire.cpp:459-501, update encode_remote_task_args and decode_remote_task_args to reject absent sidecars during both encoding and decoding; in src/common/hierarchical/remote_endpoint.cpp:739-758, update the remote TASK tensor payload-building path to require tensor_sidecar.present before constructing the payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/remote-l3-worker-design/protocol.md`:
- Around line 186-200: Update docs/remote-l3-worker-design/protocol.md lines
186-200 to remove or explicitly mark the stale metadata-only TensorWire v1
description obsolete, and consistently identify the current full TensorWire v3
format with REMOTE_SIDECAR descriptors and strided views. Update
docs/remote-l3-worker-design/implementation-plan.md line 95 to replace the “Wire
data zero” requirement with rejection tests for local backing and non-zero byte
offsets.
In `@python/simpler/remote_l3_session.py`:
- Around line 601-619: In the tensor materialization loop, validate that each
placeholder tensor’s buffer has BackendKind.REMOTE_SIDECAR before resolving or
materializing its backing. Reject any local-backed tensor even when its sidecar
is valid, and add a negative test covering that programmatically constructed
input.
In `@src/common/hierarchical/remote_wire.cpp`:
- Around line 346-375: Reject non-zero tensor byte offsets at every wire
boundary: update encode_tensor in src/common/hierarchical/remote_wire.cpp (lines
346-375) to validate tensor.byte_offset before serialization, the corresponding
C++ decoder in src/common/hierarchical/remote_wire.cpp (lines 378-422) to reject
decoded offsets, and the Python decoder in python/simpler/remote_l3_protocol.py
(lines 447-483) to reject byte_offset values. Add encode and decode coverage for
non-zero offsets in tests/ut/cpp/hierarchical/test_remote_wire.cpp (lines
89-122), ensuring invalid offsets fail during codec validation rather than later
materialization.
---
Outside diff comments:
In `@src/common/hierarchical/remote_wire.cpp`:
- Around line 459-501: Require an authoritative sidecar for every remote TASK
tensor: in src/common/hierarchical/remote_wire.cpp:459-501, update
encode_remote_task_args and decode_remote_task_args to reject absent sidecars
during both encoding and decoding; in
src/common/hierarchical/remote_endpoint.cpp:739-758, update the remote TASK
tensor payload-building path to require tensor_sidecar.present before
constructing the payload.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dcca4f4e-8b78-44b2-bbc5-918b63905fcf
📒 Files selected for processing (12)
docs/remote-l3-worker-design/buffers-and-transports.mddocs/remote-l3-worker-design/implementation-plan.mddocs/remote-l3-worker-design/protocol.mdpython/simpler/buffer.pypython/simpler/remote_l3_protocol.pypython/simpler/remote_l3_session.pysrc/common/hierarchical/remote_endpoint.cppsrc/common/hierarchical/remote_wire.cppsrc/common/hierarchical/remote_wire.htests/ut/cpp/CMakeLists.txttests/ut/cpp/hierarchical/test_remote_wire.cpptests/ut/py/test_task_interface.py
28fd56d to
f2577d2
Compare
|
@coderabbitai the outside-diff finding ("Require a sidecar for every remote TASK tensor") is fixed at the endpoint, and I left the codec alone — here is why the split.
The codec keeps the optional-sidecar encoding, because Covered by The three inline threads are answered and resolved: two fixed, one skipped with rationale. |
|
Tip For best results, initiate chat on the files or code changes.
Keeping The new 🐇 ✏️ Learnings added
You are interacting with an AI system. |
f2577d2 to
c303511
Compare
The remote L3 TASK frame carried a null-address ChipTensor per argument: the sender stripped shape and dtype out of the Tensor it already held and repacked them, and the session runner rebuilt a ChipTensor from them only to read those two fields back out. ChipTensor exists to carry a GM address, and on this wire it never did. The record is now the wire Tensor itself. The sender pushes `a.tensor(i)` verbatim; an argument bound for a remote worker is already the REMOTE_SIDECAR placeholder `TaskArgs.add_tensor` built, whose identity names the remote backing. Nothing is re-derived on either side, and the view's strides cross where before only shapes did. The sidecar stays the sole authority for an argument's backing, so a Tensor with a local backing never crosses: `backend_kind` must be REMOTE_SIDECAR and `byte_offset` must be zero, rejected on encode, on decode, and at materialization. The sidecar must also be present for every argument, not only a device-memory one -- an absent sidecar leaves the placeholder naming nothing the runner could resolve, and the endpoint refuses it before transport rather than letting it fail after decode. Orchestrator::validate_remote_sidecars already refuses a local backing alongside a sidecar at submit; the codec and the endpoint now close the same door. `remote_backing_identity` becomes the single rule for naming a backing that lives on another machine's worker, used by both the submitting L4's placeholder and the importing session runner, where the two derived it separately before. The per-argument record grows from a fixed 44 bytes to 55 + 8 * ndims (63 for a 1-D argument, 95 for a 5-D one); PROTOCOL_VERSION goes to 3, so a peer built from a different install is refused at the frame header rather than misparsing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The remote wire stops repacking what it already has
A remote L3 TASK frame carried a null-address
ChipTensorper argument. The sender pulledshape and dtype out of the
Tensorit was already holding and repacked them; the session runnerrebuilt a
ChipTensorfrom those bytes and immediately read the same two fields back out.ChipTensorexists to carry a GM address (rule 13), and on this wire it never carried one — theauthoritative backing has always been the per-argument
RemoteTensorDescsidecar.The record is now the wire
Tensoritself, pushed verbatim:An argument bound for a remote worker is already the
REMOTE_SIDECARplaceholderTaskArgs.add_tensorbuilds, whose canonical identity names the remote backing. So nothing isre-derived on either side, and the view's strides now cross where previously only shapes did.
PROTOCOL_VERSION2 -> 3. Both ends of a run come from onepip install, so the constant is amismatch alarm at the frame header, not a dual-decode selector.
The wire only accepts a backing-free argument
encode_tensor/decode_tensorboth rejectbackend_kind != REMOTE_SIDECAR, andbuild_task_payloadcarries the encoder's own guard.Orchestrator::validate_remote_sidecarsalready rejected a local backing alongside a sidecar at submit; these make it unrepresentable on
the wire as well.
Shapes and strides travel as
ndims-many entries, so the slots pastndimsthatvalidate_tensorrequires be zero are never on the wire and cannot arrive dirty.
decode_tensorrunsvalidate_tensoron every element and re-raises itsinvalid_argumentas the codec's own error.A 1-D record goes 44 -> 63 bytes; the general form is
55 + body_len + 8 * ndims. The growth isthe embedded
BufferDescriptor(identity + backing properties) that makes the recordself-describing, plus the strides that were previously dropped.
Receiving side
_materialize_task_argstakes the view from the wireTensorverbatim and the backing from thesidecar, then rebuilds a
Tensorover a backing this runner holds. Two invariants it now checks,both expressing that the sidecar is the sole authority for where the view sits:
desc.nbytesmust equal the tensor'sbuffer.nbytesbyte_offsetmust be zeroVerification
pyut— 1304 passed, 6 skipped, identical to the base commit's own runcpput— 91/91a2a3sim/a5simfull scene suites — rc=0, 0 failures on bothexamples/workers/l4/vector_add_mixed_l3on real a2a3 silicon, both halves:local max_diff=0.000e+00,remote max_diff=0.000e+00. This is the_st-pod.ymlpayload;a loopback daemon stood in for the second machine
The sim, cpput and silicon runs above were taken on the immediately preceding base
(
1f0f0060);pyutand pre-commit were re-run on this one. The rebase onto3c69de1awasconflict-free and
git range-diffreports the patch byte-identical, so nothing in the deltatouches this path.
Context
Follows #1729, which flipped
TaskArgsto carry the wireTensorbetween local processes butleft the remote hop on the old per-argument record. Strides being flattened across a remote hop
predates both and is not addressed here.