refactor(tensor_bus): PairState/BatchState are dataclasses, not Structs#108
Conversation
|
Warning Review limit reached
More reviews will be available in 15 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Failed to generate code suggestions for PR |
|
@codex reivew |
|
To use Codex here, create an environment for this repo. |
Both hold live process-group handles, tensors and CUDA work; they never cross a process boundary and can never be msgspec-encoded. The Struct base falsely signalled serializability. Only Message (the Host<->Agent command wire format) and the pure-data comm IR genuinely need msgspec. Use kw_only=True (all construction sites already pass kwargs) to guard the many no-default fields against positional mistakes.
comm.utils re-exported it from etha.pg_utils purely for backward compat. Import it directly where used (transfer.py, get_chunks.py).
50df11a to
653a9d0
Compare
What did you do
Follow-up to #106.
PairStateandBatchStatesubclassedmsgspec.Struct, but both hold liveProcessGrouphandles,torch.Tensors andBuckets carrying CUDA work. They never cross a process boundary and can never bemsgspec-encoded — theStructbase falsely signalled serializability.Across the codebase, the only genuine serialization boundary is
Message(the Host↔Agent command wire format,command_queue.py). Query/status flows transmit only scalars ("matched",bool) the Agent projects out of its in-memory state — never the state objects themselves. The pure-data comm IR (Endpoint/Route/M2MMap) is genuinely serializable and staysmsgspec.Struct.This PR:
@dataclass(kw_only=True)and fixes their docstrings (kw_onlyguards the many no-default fields against positional mistakes; all construction sites already pass kwargs; no behavior change).slotsdeliberately omitted — unlike the hot-pathChunk/Bucket, these are a handful of long-lived instances with no memory payoff.get_or_create_process_groupre-export shim fromcomm.utils; importers now pull it directly frometha.pg_utils.New test cases
None — pure type/import refactor, no new feature or bug. Covered by the existing 3-layer verification for no behavioral regression.
Test results
Other comments
The
msgspec.Structbase is now reserved for things that actually go on the wire.