Give ImportContext's capability rejections the domain-scoped vocabulary - #1782
Merged
Merged
Conversation
.docs flags this repeatedly: the "can this endpoint reach this backing" judgment exists in two independent implementations -- the domain-scoped EndpointRegistry/RegionAccessService capability engine (hw-native-sys#1696) and P1-B's ImportContext, which only ever raised freeform ValueError strings. The P1-B closure audit (2026-08-11) recorded this as a naming gap to close via "AttachmentPlan/TransportPlan 归位", scoped here to something small and safe rather than a full unification. Checked two obvious unification approaches and ruled both out, recording why on ImportContext itself so they don't get re-proposed: - Routing ImportContext's construction through the live EndpointRegistry object doesn't work across the fork boundary. ImportContext is built inside forked child processes (_sub_worker_loop, _run_chip_main_loop -- plain functions, not Worker methods) or at L2's same-process lazy-materialize point. Worker._get_endpoint_registry() walks the entire tree and requires _require_ready_for_region_planning(), a precondition that doesn't obviously hold at either of those points -- routing through it risks raising where a trivial attribute read works fine today. - Swapping ImportContext.is_host_endpoint: bool for the domain-scoped EndpointDeployment enum (HOST_CPU/DEVICE_AICORE/DEVICE_AICPU) doesn't map onto the chip-fork model: a forked chip child is one process covering both AICore and AICPU roles, and EndpointRegistry's own topology snapshot doesn't track per-chip identity anyway (_append_device_endpoint_topology passes no owner_instance_id for device entries) -- so adopting the enum wouldn't even close ImportContext's known Worker-grained-not-chip-grained limitation, it would just force an ill-fitting split for no gain. What both mechanisms already answer with the same underlying values, and can share without either problem: the vocabulary naming *why* a capability check failed. comm_endpoints.py's RegionAccessReasonCode already has UNSUPPORTED_ENDPOINT_RELATION for exactly this question. ImportRegistry. materialize()'s two DEVICE-backing rejections (host endpoint attempting a DEVICE backing; device endpoint attempting a different chip's DEVICE backing) are both instances of it. Prefixed both raised messages with the reason code's value, keeping the existing prose intact so the pre-existing test match= patterns ("host endpoint", "different chip's owner") keep matching without edits. buffer.py now imports RegionAccessReasonCode from .comm_endpoints -- checked for cycles: comm_endpoints.py imports only from _task_interface today, a fresh one-way edge. No change to ImportContext's fields, materialize()'s accept/reject logic, or anything on the hot dispatch path beyond the two error-message strings, which only build when materialize is about to raise anyway. Out of scope, both deliberately: ImportRegistry.materialize()'s backend_kind switch (the un-named adapter-selection logic for FORK_SHM/DEVICE_MALLOC/VMM_WINDOW/ POSIX_SHM) is the genuinely large, hot-path-risk piece of "AttachmentPlan/ TransportPlan 归位" and stays untouched; nothing in comm_endpoints.py itself changes. New test: both rejection messages carry RegionAccessReasonCode. UNSUPPORTED_ENDPOINT_RELATION.value, verified against the pre-fix code first (reverted the prefix, confirmed the new test fails with the plain "on a host endpoint" message and no reason code, restored the fix and confirmed it passes). The two pre-existing tests (test_host_endpoint_materialize_refuses_a_device_tensor_directly, test_chip_materialization_refuses_a_foreign_chips_device_tensor) pass unmodified, confirming the change is additive. Verified: pyut 1319 passed / 13 skipped / 0 failed; ruff check/format and pyright clean; no circular import between buffer.py and comm_endpoints.py.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds ChangesDEVICE materialization rejection codes
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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
ImportContext/ImportRegistry.materialize()(P1-B's worker-tree capability check) andEndpointRegistry/RegionAccessService(Add: CPU-NPU Comm Endpoint Model #1696's domain-scoped capability engine) are two independent implementations of "can this endpoint reach this backing" — flagged repeatedly in.docsas a naming gap, most recently in the P1-B closure audit (2026-08-11).ImportContextitself so they don't get re-proposed):ImportContext's construction through the liveEndpointRegistryobject doesn't work across the fork boundary —ImportContextis built inside forked child processes or at L2's same-process point, both of which predateEndpointRegistry's_require_ready_for_region_planning()precondition.is_host_endpoint: boolfor the domain-scopedEndpointDeploymentenum (HOST_CPU/DEVICE_AICORE/DEVICE_AICPU) doesn't map onto the chip-fork model — one forked chip process covers both AICore and AICPU roles, and the topology snapshot doesn't track per-chip identity anyway, so the enum wouldn't even closeImportContext's known Worker-grained-not-chip-grained limit.comm_endpoints.py'sRegionAccessReasonCode.UNSUPPORTED_ENDPOINT_RELATIONalready names this question. Both ofmaterialize()'s DEVICE-backing rejections now carry it, prose unchanged so existingmatch=patterns keep matching.Testing
buffer.py→comm_endpoints.pyfor import cycles: none (comm_endpoints.pyonly imports from_task_interface)