Add: CPU-NPU Comm Endpoint Model - #1696
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:
📝 WalkthroughWalkthroughThe pull request adds endpoint and backend planning, changes remote L3 transport from ChangesEndpoint planning
Host TCP runtime
Mixed-L3 validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
python/simpler/worker.py (2)
747-760: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the conflicting contexts in the error message.
_chip_descriptor_contextnow collects contexts from three sources: this worker, every local descendant, and every remote spec. When they disagree, the message does not say which values conflict or where they came from. A user who hits this on a deep L4 tree has no signal about which child or remote spec is wrong.Include the distinct contexts in the message.
♻️ Proposed diagnostic improvement
if not contexts: return "", "" first = contexts[0] if any(ctx != first for ctx in contexts[1:]): - raise RuntimeError("Worker.register: heterogeneous chip child contexts require separate callable namespaces") + distinct = sorted(set(contexts)) + raise RuntimeError( + "Worker.register: heterogeneous chip child contexts require separate callable namespaces; " + f"found {distinct}" + ) return first🤖 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 `@python/simpler/worker.py` around lines 747 - 760, Update the heterogeneous-context error in _chip_descriptor_context to include the distinct conflicting platform/runtime contexts collected from the current worker, local descendants, and remote specs. Preserve the existing validation and exception behavior while making the message identify the differing values.
2674-2709: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffCache the encoded chip payload across remote specs.
_inner_registry_entries_for_specruns once perRemoteWorkerSpec. Each call re-reads every chip blob withctypes.string_at, re-hashes it with SHA-256, re-encodes it, and hex-encodes the result. With N remote specs and M chip callables this repeats N*M full-blob copies and digests.Only
descriptordepends on the spec, throughspec.platformandspec.runtime. Remote specs commonly share those values.This cost lands on the startup path.
_open_remote_sessionderivesstartup_remaining_safter the manifest is built (Line 2752), so manifest construction is charged against the bounded startup budget.Cache the blob, its digest, and the encoded payload per
(platform, runtime)for the duration of one activation pass.🤖 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 `@python/simpler/worker.py` around lines 2674 - 2709, Update _inner_registry_entries_for_spec to use an activation-scoped cache keyed by (platform, runtime) and chip identity, storing each chip’s blob, SHA-256 digest, and encoded payload. Reuse cached values for matching remote specs, while still computing the descriptor per spec and validating it against state.descriptor; retain the existing entry construction and hex encoding behavior.tests/ut/py/test_worker/test_comm_endpoints.py (1)
92-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a mixed local and remote child case.
This test covers a remote child alone. No test covers an L4 parent that has both a local L3 child and a remote L3 child.
Both paths are formatted as
L{level}[{child_index}]under the parent, and remote children hardcodeL3. The paths stay distinct only becauseadd_workerandadd_remote_workerboth drawchild_indexfrom the single_next_level_worker_id_countcounter inworker.py. If either method ever gets its own counter, two endpoints collapse onto one path andEndpointRegistry._by_keykeeps only the last record.Add a case that calls both
add_workerandadd_remote_workeron one L4 parent, then asserts the two host paths differ and that the two children report different node scopes.🤖 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 `@tests/ut/py/test_worker/test_comm_endpoints.py` around lines 92 - 107, Extend test_remote_registry_assigns_distinct_node_scope_and_planning_rejects_cross_node, or add a focused neighboring test, to create one L4 parent with both add_worker and add_remote_worker children. Record both L3 child paths, assert their host paths differ, and verify EndpointRegistry.same_node reports different node scopes for the two children.
🤖 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 @.github/workflows/ci.yml:
- Around line 984-987: Add a workflow-level concurrency group for the
st-pod-onboard-a2a3 job, keyed to the pod pair, and set cancel-in-progress to
false so overlapping runs queue instead of terminating the active run. Keep the
existing daemon cleanup and staging TTL behavior unchanged.
- Around line 19-20: Remove the hard-coded if: false from the pre-commit
workflow job so the existing CI protection remains enabled; if temporary
pod-validation gating is required, replace it with a branch-scoped condition.
Restore detect-changes and all affected jobs, and ensure every job listing
detect-changes in needs can execute once re-enabled.
In `@docs/capability-survey.md`:
- Line 37: Align the L4 status in the capability survey with the documented CI
coverage: since line 116 says no CI job starts the daemon, mark the remote
host_tcp capability as “Shipped, not CI-run” unless daemon coverage is
confirmed; otherwise update the CI statement to reflect the verified coverage.
In `@docs/remote-l3-worker-design.md`:
- Line 410: Update the communication-policy manifest schema entry in the remote
L3 worker design so host_tcp is the only accepted value; mark roce, hccs, and ub
as reserved or future values rather than supported options, consistent with the
daemon’s current behavior.
In `@docs/remote-l3-worker-design/implementation-record.md`:
- Around line 12-20: Align the host_tcp status and scope across all three audit
documents: update the entries in
docs/remote-l3-worker-design/implementation-record.md (lines 12-20) to use the
implementation-plan status definition and explicitly list remaining gaps; revise
the implemented claims in docs/remote-l3-worker-design/implementation-plan.md
(lines 8-14) to match that record; and update
docs/remote-l3-worker-design/pr-split-and-audit-plan.md (lines 369-370) to
describe PR 6 as host_tcp scope, identifying any coverage that remains
simulation-only.
In `@examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add.cpp`:
- Around line 49-56: Run clang-format -i on kernel_entry in
examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add.cpp (lines
49-56), kernel_add_scalar.cpp (lines 51-59), kernel_mul.cpp (lines 49-56), and
the orchestration flow in
examples/workers/l4/vector_add_mixed_l3/kernels/orchestration/vector_add_mixed_l3_orchestration.cpp
(lines 42-54).
In `@python/simpler/remote_l3_session.py`:
- Around line 667-678: Update the preferred allocation path around
inner_worker.create_host_buffer and the EXPORT_BUFFER handling to preserve an
exportable descriptor for worker-owned HostBuffer instances. Store the backing
shared-memory name or host TCP descriptor in _RemoteBufferEntry rather than
relying on shm_name to interpret entry.data, and have ExportBufferResult use
that preserved descriptor while keeping the SharedMemory fallback unchanged.
In `@python/simpler/worker.py`:
- Around line 3435-3442: Update _plan_region to call _get_endpoint_registry()
once and store the result in a local registry variable, then use that same
instance for resolve_region_spec and BackendResolver construction. Keep the
existing readiness and operation-lease flow unchanged.
---
Nitpick comments:
In `@python/simpler/worker.py`:
- Around line 747-760: Update the heterogeneous-context error in
_chip_descriptor_context to include the distinct conflicting platform/runtime
contexts collected from the current worker, local descendants, and remote specs.
Preserve the existing validation and exception behavior while making the message
identify the differing values.
- Around line 2674-2709: Update _inner_registry_entries_for_spec to use an
activation-scoped cache keyed by (platform, runtime) and chip identity, storing
each chip’s blob, SHA-256 digest, and encoded payload. Reuse cached values for
matching remote specs, while still computing the descriptor per spec and
validating it against state.descriptor; retain the existing entry construction
and hex encoding behavior.
In `@tests/ut/py/test_worker/test_comm_endpoints.py`:
- Around line 92-107: Extend
test_remote_registry_assigns_distinct_node_scope_and_planning_rejects_cross_node,
or add a focused neighboring test, to create one L4 parent with both add_worker
and add_remote_worker children. Record both L3 child paths, assert their host
paths differ, and verify EndpointRegistry.same_node reports different node
scopes for the two children.
🪄 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: 25a3283f-0e80-4784-84bf-bca5f8821675
📒 Files selected for processing (25)
.github/workflows/ci.ymldocs/capability-survey.mddocs/remote-l3-worker-design.mddocs/remote-l3-worker-design/implementation-plan.mddocs/remote-l3-worker-design/implementation-record.mddocs/remote-l3-worker-design/pr-split-and-audit-artifacts.mddocs/remote-l3-worker-design/pr-split-and-audit-plan.mddocs/user/how-to/run-on-multiple-chips.mdexamples/workers/l4/vector_add_mixed_l3/README.mdexamples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add.cppexamples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add_scalar.cppexamples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_mul.cppexamples/workers/l4/vector_add_mixed_l3/kernels/orchestration/vector_add_mixed_l3_orchestration.cppexamples/workers/l4/vector_add_mixed_l3/run_parent.shexamples/workers/l4/vector_add_mixed_l3/start_machine_daemon.shexamples/workers/l4/vector_add_mixed_l3/test_vector_add_mixed_l3.pypython/simpler/__init__.pypython/simpler/comm_endpoints.pypython/simpler/remote_l3_protocol.pypython/simpler/remote_l3_session.pypython/simpler/remote_l3_worker.pypython/simpler/task_interface.pypython/simpler/worker.pytests/ut/py/test_package_surface.pytests/ut/py/test_worker/test_comm_endpoints.py
7b9df75 to
74c9abf
Compare
ccyywwen
changed the title
ChaoWao
left a comment
ChaoWao
left a comment
There was a problem hiding this comment.
Summary
The code itself is good: clean module boundaries, no dependency on the nanobind extension (pinned by a test), deterministic resolution, structured error reasons instead of string matching, and a real test per branch. If this were a greenfield endpoint helper I would approve it.
But W2's deliverable is a frozen model, and four of the frozen items disagree with domain-membership.md in ways that get expensive once W5 builds on them. Three of those four stop being redesign work and become "reuse the types the sibling PR already froze" — see below.
First: this overlaps #1599 more than it looks
domain-membership.md §3 defines the contract as
AttachmentPlan = (member EndpointId, CanonicalBufferIdentity) -> capability + attachment
This PR builds the left key. #1599 ("Add: the Buffer/Tensor wire ABI and owner-side create_buffer", open since 2026-07-30) builds the right key — the doc names it explicitly: "CanonicalBufferIdentity（wire 类型名为 CanonicalIdentity）". Neither builds the arrow; that is W5.
The two branches merge cleanly (git merge-tree → no conflicts; they touch disjoint regions of worker.py). The collision is semantic:
| Axis of the capability formula | #1599 already ships | this PR introduces instead |
|---|---|---|
| backing / backend | BackendKind {FORK_SHM, POSIX_SHM, VMM_WINDOW, REMOTE_SIDECAR, DEVICE_MALLOC, FORK_COW}, wire-frozen with static_assert |
BackingKind {DEVICE_HBM, HOST_SHM} |
| authorization | AccessMode {READ, WRITE, READWRITE}, validated on decode |
— absent |
| address space | AddressSpace {HOST, DEVICE} |
folded into EndpointDeployment |
| a capability gate | validate_buffer_descriptor, literally commented "address_space x backend_kind capability gate … (capability matrix)" |
BackendResolver, a different matrix over (deployment, deployment, same_node) |
| buffer identity | CanonicalIdentity = owner_instance_id + buffer_id + generation |
— no buffer concept |
The concrete cost. §3 gives the canonical reason backing must be in the formula:
HOST × VMM_WINDOW = ❌— 部署与互连都对，但 backing 是 VMM ⇒ host-map 仍失败
VMM_WINDOW and DEVICE_MALLOC are separate values in #1599. This PR collapses both into BackingKind.DEVICE_HBM and asks one global boolean PlatformCapability.HOST_MAP_DEVICE_HBM — so BackendResolver will emit HOST_DIRECT_MAP_ACCESS over a VMM backing, which halHostRegister rejects at the driver level (ascend_hal_base.h:2401; hardware-constraints.md §5). The distinction that makes the rule expressible exists in #1599 and is discarded here.
Suggested ordering: #1599 lands first, this PR rebases and keys its plan on BackendKind / AccessMode / CanonicalIdentity. #1599 is older, larger, and its byte layout freezes on merge; this PR is pure Python and far cheaper to move. The reverse order freezes a second backend vocabulary a week after the first, and one of them then has to be deleted.
Must fix
1. The cross-node hard-reject reintroduces a modelling error the design retracted on 2026-08-03
comm_endpoints.py:505-510 — _plan_member returns CROSS_NODE_UNSUPPORTED before consulting any capability. §4.1's rewrite note calls the old boolean form "既不准确，也制造了一个特例化的恒假分支", and §9.1 states the replacement rule directly:
请求的 adapter 不可用时才失败，而不是「因为跨机所以失败」
#1623 established a cross-host domain on two-host A3 silicon with max_diff == 0. Cross-node narrows the available adapters — C/T available, D explicitly refused with a reason — it is not a pre-emptive reject.
2. node_scope_id is derived from local-vs-remote, not node identity — and a test pins the wrong answer
_RegistryBuilder.add_worker_children allocates a fresh node_scope_id per RemoteWorkerSpec. Two live consequences:
test_comm_endpoints.py:94-98registersendpoint="127.0.0.1:1234"and assertsnot same_node(root, remote). A loopback remote worker is on the same node by construction.- Two remote L3s on one physical host get distinct scope ids and are treated as cross-node.
RemoteWorkerSpec.endpoint carries the host and is already validated to numeric IPv4 / localhost at worker.py:4116; node scope should come from there.
3. EndpointId is a bare registry-local integer, and BackendPlan carries nothing else
ResolvedRegionSpec.members keeps full records, but BackendPlan.provider_endpoint_id / ordered_member_endpoint_ids / MemberMaterialization.endpoint_id are ints only, allocated from a counter that restarts at 0 for every registry. A plan is therefore uninterpretable without the exact registry instance that produced it, and nothing binds the two — epoch sits on the registry but is not part of the id, and _record_for does not check it.
§3 requires session_id precisely "才能区分同一路径的不同 incarnation"; §10 bans the bare index ("不以「第 N 个 worker」的裸索引表达"). #1599 solves the same problem for buffers with owner_instance_id (a full-width random draw) plus generation; the same shape works here.
Related: the path root is the building worker's level, so one chip is L3/L2[0] from an L3 and L4/L3[0]/L2[0] from its parent. §3 requires worker_path to be cross-process resolvable.
4. Backing is derived solely from the MemberSet, and the flagship case cannot be expressed
_backing_for_provider maps provider deployment → one BackingKind for the whole region, and plan() opens with del layout. §7's headline row — the brick this design exists to lay ("本设计要补的那块砖") — is HOST_CPU + 同机 DEVICE_* → "host-map control Buffer + device-local/VMM payload Buffer；逐 Buffer 选 direct/copy adapter". That needs two backings and per-Buffer capability inside one domain. LayoutSummary already carries counter_bytes / payload_bytes hinting at the split, then ignores them. W2 ⑤ states the prohibition: "禁止仅从 MemberSet 推导后端".
This is the item that reduces to "use #1599's BackendKind and key capability on CanonicalIdentity".
Should fix
5. Every unsupported-plan message duplicates its endpoint label
Reproduced by running the module:
host access to device HBM is unsupported: L4 HOST_CPU: L4 HOST_CPU
cross-node region member is unsupported: L4 HOST_CPU: L4/L3[0]/L2[0] DEVICE_AICORE, L4 HOST_CPU
_plan_device_hbm_member / _plan_host_shm_member / _plan_member interpolate _endpoint_label(member), then _unsupported() appends the offending labels again (comm_endpoints.py:580-582). Drop the label from the call sites and let _unsupported own the formatting. The tests assert with in, which is why this passes.
6. Converge the worker-path format with #1599
work-breakdown.md §4 item 2 asks for exactly one thing to be shared — the format, not the storage:
只共享路径的格式/格式化工具，不共享存储或语义
The storage split is implemented correctly on both sides. The format is not: three spellings are now in flight.
| Source | Spelling |
|---|---|
this PR, EndpointRegistry |
L4/L3[0]/L2[5] |
#1599, worker.py::_create_buffer_locked |
owner_worker_path=f"L{self.level}" |
#1599, buffer.py:265 |
intern_worker_path(f"remote/{owner_worker_id}") |
f"L{self.level}" agrees with this PR's root segment by coincidence; remote/3 does not parse under _PATH_SEGMENT_RE at all. One shared format_worker_path() fixes it, and it is cheapest now — #1599's side table has no consumers yet.
7. _require_ready_for_region_planning is a weaker duplicate of the _operation_lease admission fence
worker.py:5282-5287 re-checks _lifecycle is READY with the same error string as _operation_lease (worker.py:5257), but skips the two other conditions the lease enforces — _consume_worker_host_mapped_cleanup_error_locked and _ordered_cleanup_error. It then runs twice more per call (_plan_region → _get_endpoint_registry → again), so one plan takes three _hierarchical_start_cv acquisitions. P0.2 centralised admission on the lease. Keep the static level < 3 check and drop the lifecycle branch.
8. Non-canonical adapter vocabulary
MaterializationMode.{HOST_DIRECT_MAP_ACCESS, HOST_COPY_ACCESS, DEVICE_IMPORT_ACCESS, HOST_SHM_ACCESS} against §3's "用规范的 adapter 名，不要自造": direct-map/device-peer, owner-delegated copy, explicit transfer, HCCL collective. explicit transfer — the one adapter that mints a new CanonicalBufferIdentity, and per §9.1 "在跨机上是常规手段，不是降级" — has no representation. Since W2's deliverable is the frozen vocabulary, this is the part most expensive to rename later.
9. 626 new lines, one docstring, no contract comments
The load-bearing invariants a reader cannot recover from the code are all undocumented: under excludes the path itself; overlapping at + under is an error rather than a dedupe; member order in ordered_member_endpoint_ids is selector order then (level, index) — which becomes the rank order; endpoint_id is registry-local. Per .claude/rules/comments.md these are exactly the present-tense facts worth a comment, and per doc-consistency.md §5 a frozen contract belongs in docs. Nothing in-repo records what W2 froze.
Consider
_plan_regioncan never return a supported plan in production. No production code assigns_platform_capability_cache(onlytest_comm_endpoints.py:37), andStaticPlatformCapabilityCache().getanswersFalsefor everything — so any region with ≥2 members returnsUnsupportedRegionPlan. Verified. Fine for a plan-only PR; say so in the docstring rather than leaving a working-looking entry point.- Registry epoch invalidation is unreachable. Topology freezes at
init()andCLOSEDis terminal, so no registry is ever rebuilt and a usable registry's epoch is always0. The PR body lists it as a delivered feature. EndpointRegistry.from_workerreads five privateWorkerattributes on the root and every child, typedAny. A topology-snapshot accessor onWorkerwould keepworker.pyowning its own shape.EndpointSelectoris exported and directly constructible, bypassingat()/under()validation (test_comm_endpoints.py:47does this). Validate in__post_init__or drop it from__all__.- §5/§6 encapsulation. The root builds one global registry including remote children's
spec.device_ids, whereas §6 wants subtree selectors expanded "各 L3 本地展开" so "L4 不必知道 chip 拓扑". Today'sRemoteWorkerSpecalready hands the root that list, so this PR is not inventing the knowledge — but if local expansion is still the intent, this is the moment to say so.
CI and stale feedback
st-onboard-a5 is red, and it is not this PR. The single failure is
FAILED tests/st/a2a3/tensormap_and_ringbuffer/spmd_paged_attention_highperf/
::TestSpmdPagedAttentionHighPerf::test_run - RuntimeError: run failed with code -100
a device-side AICPU rc=-100. This diff is pure Python — no C++, no kernel, no scheduler — and the only behaviour change in worker.py outside new methods is one _invalidate_endpoint_registry() call in close(). The same test has prior history with this signature (#1070). I have re-run the failed job; if it reproduces it needs its own triage rather than silence.
The 9 existing CodeRabbit items on this PR are stale. All 7 inline threads are isOutdated and anchored to files absent from this 5-file diff (.github/workflows/ci.yml, docs/remote-l3-worker-design/*, examples/workers/l4/vector_add_mixed_l3/**, python/simpler/remote_l3_session.py), and the review body reviews _chip_descriptor_context / remote-spec encoding. They predate the branch reset onto c866c827; CodeRabbit has not re-reviewed since (Review skipped: incremental reviews are disabled). Nothing there needs answering.
Verification note. I could not run the new tests locally — the build-stamp guard correctly refuses a _task_interface from another worktree, and a fresh editable install fails on missing nanobind (a local setup gap, unrelated to this PR). CI's ut job is green on both ubuntu and macOS and covers exactly these two files. The selector semantics, the default-cache planning outcome, and the duplicated-label defect above were verified by importing comm_endpoints.py directly, which needs no extension.
c719170 to
edd611e
Compare
|
Thanks @ChaoWao for the detailed review. The PR has been updated at commit Current status: this PR is still Python planning-only. It freezes the W2 Response structure: the first section covers the #1599 overlap and vocabulary Overlap With #1599Accepted. W2 no longer defines a second backing vocabulary.
Must Fix1. Cross-node hard rejectResolved. The resolver no longer returns a cross-node-specific failure before The failed candidates are reported through 2.
|
|
W2 follow-up update: commit Key changes:
Tests added/updated cover:
Validation: Result: |
bbc3fd6 to
24370ad
Compare
ChaoWao
left a comment
ChaoWao
left a comment
There was a problem hiding this comment.
Second-round review — 24370ad7
Thanks for the rework. 9 of the 11 findings from round 1 are fixed, and one came back better than I asked for: turning the capability lookup table into a capability service (RegionAccessService) is the right shape. A query that carries both endpoint records plus the backend is not just tidier — it is the correct shape for a reason that turns out to be bigger than this PR, see below.
Fixed since round 1: cross-node hard reject · node identity · EndpointIdentity · duplicate labels · path format · admission fence · adapter vocabulary · from_worker private-attribute access · EndpointSelector bypass. EndpointId now carries session_instance_id, which closes the deviation I recorded on 08-05.
CI is green (19 checks). Requesting changes on three blockers.
The organising principle, restated — this changed how I read the PR
Our design guideline was restated today, and it reframes both this PR and two of the blockers below. Quoting it because it is not in this repo:
Physical resources (memory, communication domains) are decoupled from the logical concept (Worker).
Whether a worker may hold or access a resource is decided by where it is deployed × the hardware connectivity from there to where the resource physically lives.
A worker's position in the logical tree (level / worker_path) decides only who may initiate, never whether it is allowed.
The consequence for this PR: capability(endpoint_deployment, resource_physical_identity) → allowed? is one function, and it currently appears in five places in the repo — RegionAccessService (at planning time, this PR) · ImportContext / materialize (at materialization time, P1-B) · AttachmentPlan (at domain construction) · TransportPlan adapter selection · comm_profile (cross-node consistency assertion). Those are not five similar things. They are one function evaluated at five moments.
Two decisions follow, both taken today by the maintainer:
Decision ① — the single authority for that judgement is this PR's registry. ImportRegistry.materialize in P1-B needs an endpoint × address_space gate to enforce "a device reference only reaches its owner chip". That is the same judgement RegionAccessService already computes. So W2's registry is the sole authority and ImportContext becomes a thin consumer of it — P1-B will not build a second matrix.
This is not "two teams collided, pick an owner". It is forced: the information the judgement needs is not on the wire.
| field | why it is not enough |
|---|---|
owner_instance_id |
opaque nonce, carries no location |
owner_worker_path_id |
by design "DIAGNOSTIC id only … takes part in no routing, visibility or identity decision" |
address_space |
only HOST / DEVICE — does not say which card |
Only a registry that knows deployment can supply it.
What this adds here: one binding table, owner_instance_id → owner endpoint, so a consumer holding a BufferDescriptor can resolve which endpoint minted it. Small, but it is what makes single-authority real rather than nominal.
Decision ② — session_instance_id and owner_instance_id stay shared, and the sharing is documented as deliberate, because given ① it is the bridge: BufferDescriptor.identity.owner_instance_id is the key into that binding table. See blocker 1.
Blockers
1. _owner_instance_id is assigned twice, at two different widths, and the second one is load-bearing
In Worker.__init__ the field is written first as uuid.uuid4().bytes (16 bytes, added by this PR), then overwritten by mint_owner_instance_id() (8 bytes, os.urandom(OWNER_INSTANCE_ID_BYTES), from #1599). The first line is dead — but the width is wrong, and #1599's docstring states that width is load-bearing ("Must stay a full-width random draw"). A reader who deletes the second line to remove the apparent duplication silently widens the nonce and breaks that contract.
Please delete the uuid.uuid4().bytes line, and per decision ② add a comment on the remaining one stating that EndpointIdentity.session_instance_id deliberately shares this nonce with BufferDescriptor.identity.owner_instance_id, because that is the key used to resolve a descriptor back to its minting endpoint.
Why a comment and not just a deletion. There is an open gate (G4) that will move the mint point to after fork/adoption. When it lands, every EndpointIdentity minted before it changes meaning — and stable identity across incarnations is exactly what this PR is for. It does not need solving here: EndpointIdentity currently never crosses a process boundary and is never serialized, so the coupling is latent, not live. But it has to be written down, or whoever closes G4 will not know it reaches W2.
2. Both region parts get one BackendKind, and the resulting plan is one that main's C++ validator already rejects
_backend_kind_for_provider hands both parts of a device provider VMM_WINDOW. _adapter_candidates then, underneath that VMM_WINDOW, offers DIRECT_MAP / HOST_SVM_MAP for the COUNTER part.
Three independent sources say that is illegal:
- The capability matrix (
p1b-corrected-design.md§5.1) hasHOST × VMM_WINDOW = ❌, andmainalready enforces it in C++:validate_buffer_descriptorraises "unsupported address_space x backend_kind (capability matrix)". - Hardware:
halHostRegisteris documentedNot support vmm va(CANN 9.0,ascend_hal_base.h). One backing cannot be both VMM peer-imported and host-registered. - Invariant 5 (a device reference only reaches its owner chip).
So the Python planner can emit a plan C++ on main will refuse, and test_static_service_can_select_counter_direct_map_without_payload_direct_attempt pins that plan as expected behaviour. Nothing breaks today only because NO_IMPLEMENTED_DIRECT_MAP_PROBE sits in front of it — a probe stub, not a guarantee.
This is the principle above, in miniature: the plan was derived without consulting what the physical backing can actually do. The intended shape is already written down in hardware-constraints.md §5 — split into a control_buffer (host-mappable, non-VMM backing) and a payload_buffer (device VMM), choosing the adapter per buffer. The counter is the control buffer; its backing cannot be VMM_WINDOW. Either take part into account in _backend_kind_for_provider, or drop HOST_SVM_MAP from the counter candidates under VMM_WINDOW. The test has to change either way, since it currently asserts the illegal plan.
3. comm_endpoints became a hard dependency on the extension, and the test guarding that was deleted with no replacement
from _task_interface import BackendKind is a module-level import. The previous revision carried test_comm_endpoints_import_survives_without_the_extension, docstring "The W2 endpoint model is pure Python and must not import _task_interface". That test is gone in 24370ad7 and nothing replaced it.
To be clear: using the canonical BackendKind is correct — I asked for it in round 1. The problem is that a contract changed with no record. simpler/__init__.py still lists comm_endpoints in _LAZY_SUBMODULES, and laziness only defers the failure, it does not remove it. Either restore a test asserting the new contract (importing comm_endpoints requires the extension, and the failure is legible), or drop it from _LAZY_SUBMODULES and say in the module docstring that it is now extension-dependent. Deleting the guard while leaving the lazy registration is the one combination that records nothing.
One question, not a blocker
_require_ready_for_region_planning still carries if int(worker.level) < 3: raise, and it now covers planning as well as registry construction. Under the principle above, level may decide who may initiate but never whether it is allowed — domain-membership.md §2.3 says as much, while §2.4 says control-tree visibility is level's business.
I read your gate as the §2.4 kind. If that is right, one comment saying so closes this permanently and I will stop raising it. If it is ever consulted to decide reachability, it becomes the §2.3 violation.
Where this PR now sits
Stating it explicitly, because neither side knew until today: RegionAccessQuery carries two endpoint records plus the backend — exactly the endpoint × address-domain context ImportRegistry.materialize is missing. Before decision ①, the repo was on track to grow two endpoint matrices, one deciding "may this be planned" and one deciding "may this be materialized", free to disagree and very hard to merge once both had tests.
Practically: after the three blockers, this PR is the head of a short chain — #1696 → add the deployment dimension to the domain member struct (#1623/#1624 are held until then, because that struct is already on the cross-node wire) , and separately #1696 → G4 → ImportContext. Nothing for you to act on there; I mention it so the binding table in decision ① does not read as scope creep.
24370ad to
0b59501
Compare
|
@ccyywwen Thanks — the rework answered round 1 well, and the capability service is a better shape than the lookup table I asked for. I've pushed the remaining pieces onto this branch rather than send you round-tripping on them; the branch is now one squashed commit on current Blocker 2 — direct-map over a VMM backing. Dropped I took the weaker of the two options the review offered, deliberately. The right fix is the one The exclusion lives in candidate enumeration rather than in a Blocker 1 — the duplicate nonce. Deleted the 16-byte Blocker 3 — the extension contract. Added Decision ① — the binding table. Implemented. Each Worker's topology entry now carries the buffer-owner nonce it mints under, and
The level gate. Your reading was right, and it now says so in a docstring: Verification: One note on the squash: your six commits are folded into one. The separation you had — vocabulary, then the access service — was legible, and I would normally leave it; a single commit is this repo's default for a PR and that is the only reason. |
ChaoWao
left a comment
ChaoWao
left a comment
There was a problem hiding this comment.
Approving — all three blockers are closed, and two of them came back stronger than asked
Verified on 0b59501b by reading the code, not the commit message.
Blocker 1 — the double _owner_instance_id assignment: fixed. One mint point remains. The comment does more than record the deletion: it states why the sharing with EndpointIdentity.session_instance_id is load-bearing (it is the key of the owner_instance_id -> owner endpoint binding, and the descriptor cannot name its own minting endpoint because the nonce is opaque, owner_worker_path_id is diagnostic by contract, and address_space does not say which card), and it states the consequence for the future mint-point move — that endpoint identity and buffer identity have to move in one change. That is exactly the record I asked for, and the reasoning is now in the file rather than in a review thread.
Blocker 2 — the illegal HOST × VMM_WINDOW plan: fixed, and fixed at the right layer. The direct-map candidate is gone from enumeration rather than from a service verdict, with the CANN reason (Not support vmm va) cited in place. The comment explains why enumeration is the right layer: a plan an injected service accepted could not be materialized, so the exclusion must not be overridable.
The test change is what makes this convincing. test_host_direct_map_is_never_offered_over_a_vmm_backing injects a deliberately permissive service — one that would happily admit the pairing — and then asserts the candidate never reaches it. That proves absence from enumeration rather than rejection downstream, which a straightforward assertion could not distinguish. test_default_service_refuses_host_svm_map_when_asked_directly is kept as the backstop with a docstring saying why both exist. This is a stronger construction than the fix I suggested.
Blocker 3 — the deleted extension guard: fixed. test_comm_endpoints_requires_the_extension_and_stays_lazy pins the new contract in a subprocess with _task_interface poisoned: import simpler still survives, reaching simpler.comm_endpoints raises. It also asserts ce.BackendKind is _task_interface.BackendKind, which pins the single-vocabulary property that motivated the change. The docstring records that the old property was traded and what replaced it — that was the actual gap, not the import itself.
Decision ① — the binding table is there. EndpointRegistry.owner_endpoint(owner_instance_id) plus the owner_bindings construction path, and a duplicate nonce raises rather than silently binding to the last writer. A remote Worker's nonce is left absent rather than guessed, which is the right call — guessing it would have made the table lie for exactly the cross-node case it exists to serve.
The level < 3 question is answered. The docstring now says it is control-tree visibility, not a capability decision, and names what a capability decision would actually consult (deployment, interconnect, backing, live attachment — none of which appear there). Closing that one for good.
CI: 18 green, 1 skip.
One thing before merge: rebase, and be careful with the single conflict
main moved to aa1d7c7d (#1729, the Tensor wire flip) after this head was pushed. This PR now conflicts on one file, one hunk: python/simpler/worker.py.
The hunk is a relocation, not a removal — #1729 moved self._owner_instance_id = mint_owner_instance_id() to a different point in Worker.__init__ (now around worker.py:3881). Nothing was deleted.
The correct resolution is to keep #1729's single assignment at its new location and move this PR's comment block onto it. Nothing else in the hunk changes.
Approving on the current content; the rebase is mechanical, so I am not asking for another round — please just double-check that one hunk and confirm CI stays green after the rebase.
A domain's members are named today by bare chip index, which binds membership
to one level of the tree and cannot express a host process and a device view of
the same card as two distinct participants. This lands the endpoint model that
replaces it — an endpoint identity, a selector language, and a plan that picks a
transport adapter per region part — in Python only, with no materializer and no
wire ABI.
An endpoint is a `worker_path` plus a `deployment` (`HOST_CPU`,
`DEVICE_AICORE`, `DEVICE_AICPU`), so one path can carry several deployment
views of the same hardware. Paths are written by one helper and parsed by one
grammar (`L4/L3[0]/L2[5]`), shared with the buffer layer's diagnostic owner
path so the two cannot drift on spelling while keeping their storage separate.
`EndpointIdentity` carries `session_instance_id` and `registry_epoch` alongside
the registry-local id, so an id from a previous incarnation cannot silently
resolve against a later registry. The session nonce is the Worker's buffer-owner
nonce: sharing it is what lets `EndpointRegistry.owner_endpoint` turn the
`owner_instance_id` of a `BufferDescriptor` back into the endpoint that minted
it. The descriptor cannot answer that itself — the nonce is opaque,
`owner_worker_path_id` is diagnostic by contract, and `address_space`
distinguishes only HOST from DEVICE, never which card — so the registry is the
only place the judgement can live. A nonce minted in another process resolves to
a typed refusal rather than a guess.
`at()` names one endpoint, `under()` expands a subtree, and overlapping
selectors are refused rather than silently deduplicated. Node scope comes from
the normalized host of each Worker's endpoint, so a loopback remote worker
shares a node with its parent and two remote L3s on one host share a scope —
being remote is not the same fact as being on another node.
Reachability is not a property of a member set. `RegionAccessService` decides
per `(part, backing, provider endpoint, consumer endpoint)` and records every
refusal as an `AdapterAttempt`, so an unsupported region reports which adapters
were tried and why each failed. Cross-node members resolve normally and then
narrow to the adapters that can serve them, rather than failing for being
cross-node.
Adapters use the canonical names — direct map, device peer, owner-delegated
copy, explicit transfer, collective — with the implementation profile kept as a
separate axis, so "which adapter" and "how it is realized" stay independent.
Two constraints are structural and therefore live in candidate enumeration
rather than in a service verdict, which an injected service could override:
- No host direct-map candidate exists over a `VMM_WINDOW` backing.
`halHostRegister` refuses a VMM VA ("Not support vmm va", CANN 9.0
`ascend_hal_base.h`), so one backing cannot be both VMM peer-imported and
host-registered, and a plan selecting it is one no materializer can honour.
A host-mappable control buffer needs its own non-VMM backing, which no
platform path allocates yet; `_backend_kind_for_provider` records why both
parts share one backing until one does.
- Region planning admits only a Worker that owns a control subtree. That is
control-tree visibility — who may *declare* a region — and never a decision
about who may share memory, which no level participates in.
No `create_region`, no materialization, no C++ or wire changes. Capability
probing is not integrated, so the default service supports only same-node host
access to a device backing; everything else reports the adapter it would need.
`comm_endpoints` takes `BackendKind` from the extension rather than mirroring
it, so there is one backing vocabulary rather than two — which makes the module
extension-dependent, and a test pins that contract in both directions.
0b59501 to
cdccc1d
Compare
…ry (#1782) .docs flags this repeatedly: the "can this endpoint reach this backing" judgment exists in two independent implementations -- the domain-scoped EndpointRegistry/RegionAccessService capability engine (#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.
2 participants
Summary
Implements the cpu-npu-shared-memory endpoint model in Python only.
simpler.comm_endpointswith endpoint selectors, path parsing,endpoint registry resolution, node-scope relation queries, capability cache
stubs, and
SingleOwnerbackend planning.Worker._resolve_region_spec(...)andWorker._plan_region(...)with lazy endpoint registry construction andregistry epoch invalidation on close.
provider resolution, same-node/cross-node behavior, backend planning, and
package import surface.
Scope
This PR intentionally does not add
Orchestrator.create_region(...), does notmaterialize regions, and does not touch C++ / nanobind / wire ABI.
HOST_MAP_DEVICE_HBMremains a capability contract only. Real probing must validate bidirectional host-device counter visibility.Tests
pytest tests/ut/py/test_worker/test_comm_endpoints.py tests/ut/py/test_package_surface.pypytest tests/ut/py/test_worker/test_l4_recursive.py tests/ut/py/test_worker/test_l3_l2_orch_comm.py tests/ut/py/test_worker/test_comm_endpoints.py