Rename: simpler_setup.Tensor -> simpler_setup.TensorArg - #1741
Merged
ChaoWao merged 1 commit intoAug 8, 2026
Conversation
simpler_setup.Tensor (a NamedTuple test-arg spec used by TaskArgsBuilder) and simpler.task_interface.Tensor (the wire-ABI struct PR hw-native-sys#1729 wired up) now name two unrelated public types the same thing in two importable namespaces, violating codestyle.md rule 13 (public C++/Python types share one canonical name and semantics). hw-native-sys#1729 landed with zero Python call sites on the ABI Tensor, so renaming the long-established, 119-call-site test-tooling type is the low-risk direction: it is a mechanical rename with no behavior change, versus fighting the design docs' canonical name for the wire element. Renamed the class in simpler_setup/scene_test.py, its export in simpler_setup/__init__.py, and every constructor/isinstance call site across examples/, tests/, and docs/testing.md, docs/user/README.md, .claude/rules/project-layout.md. docs/user/README.md's "one name to watch" caveat about the collision is removed since it no longer applies. Verified: ruff check/format clean, pyut 1251 passed / 13 skipped / 0 failed, and a live a2a3sim run through TaskArgsBuilder/compute_golden confirms the renamed spec still round-trips through the harness.
|
Important Review skippedToo many files! This PR contains 126 files, which is 26 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (126)
You can disable this status message by setting the 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 |
6 tasks
ChaoWao
added a commit
that referenced
this pull request
Aug 8, 2026
PR #1729 (wire flip, merged aa1d7c7) deliberately deferred 6 cleanup items to its own PR comments rather than growing an already 126-file change. Item 6 (the simpler_setup.Tensor / simpler.task_interface.Tensor naming collision) was closed separately by #1741. This closes 4 of the remaining 5; the 5th needs a C++ signature change and is left for a follow-up (see below). MappedArg.buffer ignored the descriptor's access mode and always returned a writable memoryview, including for FORK_COW backings, whose whole contract is that a write is invisible to the owner (copy-on-write splits the page privately). A callable that wrote through it lost data silently. buffer now returns a read-only view (memoryview.toreadonly()) when access is AccessMode.READ. New test: test_mapped_arg_buffer_is_read_only_for_a_read_access_descriptor (fails on the old code, passes after the fix). Note: torch.frombuffer does not itself honor a read-only memoryview -- it only warns and still allows the write -- so this closes the contract at the buffer-protocol layer; it does not stop a torch consumer from writing through its own tensor view. Fixing that would need actually protecting the COW pages (e.g. mprotect), out of scope here. ImportRegistry.materialize_blob and .materialize_args each rebuilt a snapshot of every identity the endpoint had ever materialized (self._by_identity in full) on every dispatch, via a now-deleted materialization_map() helper -- O(every buffer this chip child has ever seen) instead of O(this task's own tensor count), on the chip and L2-leaf dispatch path. Both callers only ever look up entries for tensors they independently re-parsed from the same blob/TaskArgs, so no entry outside the current call's own tensors was ever consulted. Both methods now build their returned dict directly from their own loop. New test: test_materialize_args_scopes_the_returned_map_to_this_calls_tensors, asserting a second call's returned map does not carry a first call's identity forward. ImportRegistry.unregister had zero callers and zero test references anywhere in the repo. The "import mapping released with handle lifecycle" invariant it was meant to serve doesn't have a lifecycle to attach to yet -- release_buffer() doesn't exist in this codebase. Deleted rather than left as an untested stand-in for a feature that isn't built. tests/st/{a2a3,a5}/tensormap_and_ringbuffer/test_l3_host_buffer_registration.py were still named after register_host_buffer, deleted long ago -- only the filename was a leftover; the class name (TestPostForkHostBufferZeroCopy) and docstrings already describe the current create_buffer + POSIX-shm mechanism accurately. Renamed both arch siblings in this commit to test_l3_post_fork_host_buffer.py. Also fixed a now-stale cross-reference in .docs/l3l4/memory-kinds.md and flagged (but did not chase down) an unrelated pre-existing gap it also pointed at: the ut test it names for kind3 registration no longer exists in the repo. Deferred to a follow-up PR: the chip task blob gets decoded twice on every dispatch -- once by read_args_from_blob (to drive ImportRegistry.materialize) and again by materialize_tensor_blob's own C++ read_blob call, on the same bytes. Closing that needs materialize_tensor_blob to accept the already-parsed view instead of re-reading raw bytes, which is a signature change on python/bindings/task_interface.cpp's hot dispatch path -- it deserves its own PR with dedicated dispatch-latency verification rather than riding along with these four independent one-line fixes. Verified: pytest tests/ut 1281 passed / 13 skipped / 0 failed; ruff check/format clean; a real a2a3 onboard run (test_l3_tensor_dispatch.py, 2 devices) exercising the changed chip-dispatch materialize path; test_l3_post_fork_host_buffer.py passing under its own a2a3sim platform restriction on both arch siblings.
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.
Summary
simpler_setup.Tensor(theNamedTupletest-arg spec used byTaskArgsBuilder) andsimpler.task_interface.Tensor(the wire-ABI struct Update: cut task args over to the self-describing Tensor wire ABI #1729 finished wiring up) name two unrelated public types the same thing in two importable namespaces — a violation of.claude/rules/codestyle.mdrule 13 (public C++/Python types share one canonical name and semantics), flagged as a deferred follow-up in Update: cut task args over to the self-describing Tensor wire ABI #1729.Tensor, so renaming the long-established, 119-call-site test-tooling type is the low-risk direction — a mechanical rename with no behavior change, rather than fighting the design docs' canonical name for the wire element.simpler_setup/scene_test.py, its export insimpler_setup/__init__.py, and every constructor/isinstancecall site acrossexamples/,tests/, plus prose references indocs/testing.md,docs/user/README.md, and.claude/rules/project-layout.md.docs/user/README.md's "one name to watch" caveat about the collision is removed since it no longer applies.Test plan
ruff check/ruff format --checkclean onsimpler_setup/,examples/,tests/pytest tests/ut— 1251 passed, 13 skipped, 0 failedpytest examples/a2a3/tensormap_and_ringbuffer/vector_example/test_vector_example.py --platform a2a3sim— 1 passed--platform a2a3simsweep acrossexamples/a2a3andexamples/a5(the sim-eligible subset) — all green