Forward skip_tiling across the controller boundary - #857
Open
lokic233 wants to merge 1 commit into
Open
Conversation
The remote-coordination branch of `_execute_slice_broadcast`
(`tpu_sync/rpc/raiden_controller.py:1569`) forwards `skip_d2h=plan.skip_d2h`
to `register_transfer_schedule` but does not forward `plan.skip_tiling`.
Both flags configure how the host staging buffer is interpreted, and they are
set together. The sender's D2h does see the map -- it reaches the local source
worker through the `start_transfer(s_node, plan)` call just below -- so after
the boundary the sender detiles according to a populated `skip_tiling` while
the receiver's H2d re-tiles according to an empty one. Nothing catches the
disagreement: no bounds check fires, the chunk counts still match, the
transfer reports success, and the landed weights are silently wrong.
Adds the missing kwarg plus a regression test that records the kwargs handed
to a faked `RaidenControllerClientFacade` and asserts `skip_tiling` arrives
intact alongside `skip_d2h`.
Measured, on an unmodified tree:
without the fix the new test fails, AssertionError: None != {7: True, 9: False}
rest of raiden_controller_test.py: 35/35 pass
with the fix the new test passes
raiden_controller_test.py: 36/36 pass
The test discriminates the one changed line rather than the setup: its
`assertTrue(kwargs.get("skip_d2h"))` assertion passes on both arms, and only
the `skip_tiling` assertion moves.
Scope note, deliberately narrow: this is a forwarding omission found while
investigating a separate replicated-leaf drop. It is **not** claimed to be the
cause of that drop -- that root cause is still unknown, and the earlier
hypotheses (collision, rank-1 tiling, C++ `(slice, dst_device)` keying) were
each tested and refuted. This change stands on its own.
Signed-off-by: Loki Chen <dengcchi@meta.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.
What
Forwards the missing
skip_tilingkwarg across the controller boundary, plus aregression test.
Why
The remote-coordination branch of
_execute_slice_broadcast(
tpu_sync/rpc/raiden_controller.py:1569) forwardsskip_d2h=plan.skip_d2htoregister_transfer_schedulebut does not forwardplan.skip_tiling.Both flags configure how the host staging buffer is interpreted, and they are set
together. The sender's D2h does see the map — it reaches the local source worker
through the
start_transfer(s_node, plan)call just below — so after the boundary thesender detiles according to a populated
skip_tilingwhile the receiver's H2d re-tilesaccording to an empty one.
Nothing catches the disagreement: no bounds check fires, the chunk counts still match,
the transfer reports success, and the landed weights are silently wrong.
Testing
New regression test records the kwargs handed to a faked
RaidenControllerClientFacadeand assertsskip_tilingarrives intact alongsideskip_d2h. Measured on an unmodified tree:AssertionError: None != {7: True, 9: False}; rest ofraiden_controller_test.py35/35 passraiden_controller_test.py36/36 passThe test discriminates the one changed line rather than the setup: its
assertTrue(kwargs.get("skip_d2h"))assertion passes on both arms, and only theskip_tilingassertion moves.Scope note — deliberately narrow
This is a forwarding omission found while investigating a separate replicated-leaf drop.
It is not claimed to be the cause of that drop. That root cause is still unknown, and
the earlier hypotheses (collision, rank-1 tiling, C++
(slice, dst_device)keying) wereeach tested and refuted. This change stands on its own merits.