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
| pre-commit: | ||
| if: false |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
Do not merge with every pre-existing job disabled.
if: false now disables pre-commit, packaging-matrix, ut, st-sim-a2a3, st-sim-a5, profiling-flags-smoke, ut-a2a3, st-onboard-a2a3, detect-changes, ut-a5, and st-onboard-a5. Only st-pod-onboard-a2a3 remains active. On the default branch this removes all lint, unit-test, packaging, and scene-test protection.
If the disabling is temporary scaffolding for pod validation, gate it on the branch instead of hard-coding false, and restore the jobs before merge. Example gate:
♻️ Branch-scoped gate instead of a hard disable
pre-commit:
- if: false
+ if: github.ref_name != 'pod-ci' && github.ref_name != 'ci-multiserver-validation'Note also that the disabled detect-changes job is still listed in needs: for several jobs. Once those jobs are re-enabled, detect-changes must be re-enabled too, otherwise they stay skipped.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pre-commit: | |
| if: false | |
| pre-commit: | |
| if: github.ref_name != 'pod-ci' && github.ref_name != 'ci-multiserver-validation' |
🤖 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 @.github/workflows/ci.yml around lines 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.
|
|
||
| ssh "${SSH_OPTS[@]}" "$REMOTE_TARGET" " | ||
| pkill -f 'python -m simpler.remote_l3_worker --host ${REMOTE_DAEMON_HOST} --port ${REMOTE_DAEMON_PORT}' || true | ||
| " || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Scope this job to one run at a time.
The pkill -f pattern matches only the daemon host and port, not the run. REMOTE_DAEMON_PORT comes from POD_L3_DAEMON_PORT, which is fixed per machine pair. If two runs of st-pod-onboard-a2a3 overlap on the same pod pair, the second run's pre-start pkill at line 986 terminates the first run's live daemon, and both runs contend for the same listening port. The workflow triggers on pushes to two branches, so overlapping runs are reachable.
Add a concurrency group so the pod pair serves one run at a time.
🛡️ Proposed fix
st-pod-onboard-a2a3:
runs-on: [self-hosted, Linux, ARM64, pod-a2a3]
timeout-minutes: 60
+ concurrency:
+ group: st-pod-onboard-a2a3
+ cancel-in-progress: false
env:Note that cancel-in-progress: false is required here. A cancelled run is SIGKILLed and never reaches its EXIT trap, which is the case the staging TTL sweep at line 959 already accounts for.
🤖 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 @.github/workflows/ci.yml around lines 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.
| The 7-level model (L6 Cluster … L0 Core) is declared in | ||
| [hierarchical-level-runtime.md](hierarchical-level-runtime.md). Its own status | ||
| table is accurate: L3 implemented; L4 local implemented, remote simulation only; | ||
| table is accurate: L3 implemented; L4 local implemented, remote host_tcp shipped; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the L4 status with the documented CI coverage.
Line 27 defines Shipped as covered by CI. Line 116 states that no CI job starts the daemon. Either use Shipped, not CI-run, or update the CI statement after confirming daemon coverage.
🤖 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 `@docs/capability-survey.md` at 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.
| hashid -> ChipCallable register payload, when needed | ||
| hashid -> Python import descriptor, when needed | ||
| comm policy: roce | hccs | ub | sim | ||
| comm policy: host_tcp | roce | hccs | ub |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mark future profiles as unsupported in the manifest schema.
Line 410 lists host_tcp | roce | hccs | ub as communication-policy options. The daemon currently rejects every profile except host_tcp, as documented in docs/capability-survey.md Lines 110-114. Show only host_tcp as accepted in this cut. List the other profiles as reserved or future values.
🤖 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 `@docs/remote-l3-worker-design.md` at 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.
| | 1 | Endpoint interface and local adapter | In progress | Local adapter and remote host_tcp endpoint are implemented; HCOMM endpoint adapters remain. | | ||
| | 2 | Worker eligibility metadata | In progress | Callable worker-id sets are intersected with owner/imported remote sidecar eligibility. | | ||
| | 3 | Remote task sidecars and dependency keys | In progress | Public `TaskArgs.add_tensor(RemoteTensorRef(...))` API, remote TensorMap keys, and remote payload-sidecar rejection are implemented. | | ||
| | 4 | Failed task poisoning | In progress | Remote task-failure poisoning and session-exit endpoint failure are verified; explicit health-expiry-only coverage remains. | | ||
| | 5 | Versioned remote frame codec | In progress | TASK/COMPLETION/CONTROL_REPLY/HELLO/CONTROL/HEALTH exist; core fuzz/bounds coverage is present, with more exhaustive corpus testing still possible. | | ||
| | 6 | Remote callable registry | In progress | Dispatcher `PYTHON_IMPORT`, inner manifest/control `PYTHON_IMPORT`, and inner manifest/control inline `CHIP_CALLABLE` are implemented; serialized payloads and staged chip blobs remain negotiated extensions. | | ||
| | 7 | Fork-safe simulation session runner | In progress | Daemon/session bootstrap and HELLO READY barrier are implemented for sim transport. | | ||
| | 8 | Remote control-plane parity | In progress | Registry, alloc/free/copy, export/import/release-import controls are implemented for sim; Remote CommDomain controls are reserved/unsupported. | | ||
| | 9 | Remote buffer registry | In progress | Sim owner/imported buffers, TASK materialization, public memory API, opaque handles, slot/import-ref capture, and deferred free/release-import are implemented. | | ||
| | 7 | Fork-safe host_tcp session runner | In progress | Daemon/session bootstrap and HELLO READY barrier are implemented for host_tcp transport. | | ||
| | 8 | Remote control-plane parity | In progress | Registry, alloc/free/copy, export/import/release-import controls are implemented for host_tcp; Remote CommDomain controls are reserved/unsupported. | | ||
| | 9 | Remote buffer registry | In progress | host_tcp owner/imported buffers, TASK materialization, public memory API, opaque handles, slot/import-ref capture, and deferred free/release-import are implemented. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align host_tcp status and PR scope across the audit documents.
The documents use conflicting completion and scope labels for the same runtime cut. Use one status definition, state remaining gaps, and remove stale simulation terminology.
docs/remote-l3-worker-design/implementation-record.md#L12-L20: align theIn progressentries with the implementation plan and list the remaining gaps.docs/remote-l3-worker-design/implementation-plan.md#L8-L14: align theimplementedclaims with the implementation record.docs/remote-l3-worker-design/pr-split-and-audit-plan.md#L369-L370: rename the PR 6 scope from simulation to host_tcp and identify any simulation-only coverage.
📍 Affects 3 files
docs/remote-l3-worker-design/implementation-record.md#L12-L20(this comment)docs/remote-l3-worker-design/implementation-plan.md#L8-L14docs/remote-l3-worker-design/pr-split-and-audit-plan.md#L369-L370
🤖 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 `@docs/remote-l3-worker-design/implementation-record.md` around lines 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.
| extern "C" __aicore__ __attribute__((always_inline)) void kernel_entry(__gm__ int64_t *args) { | ||
| // Unpack arguments (Tensor* pointers from runtime) | ||
| __gm__ Tensor *src0_tensor = reinterpret_cast<__gm__ Tensor *>(args[0]); | ||
| __gm__ Tensor *src1_tensor = reinterpret_cast<__gm__ Tensor *>(args[1]); | ||
| __gm__ Tensor *out_tensor = reinterpret_cast<__gm__ Tensor *>(args[2]); | ||
| __gm__ float *src0 = reinterpret_cast<__gm__ float *>(src0_tensor->buffer.addr) + src0_tensor->start_offset; | ||
| __gm__ float *src1 = reinterpret_cast<__gm__ float *>(src1_tensor->buffer.addr) + src1_tensor->start_offset; | ||
| __gm__ float *out = reinterpret_cast<__gm__ float *>(out_tensor->buffer.addr) + out_tensor->start_offset; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Run the required C++ formatter before merge.
examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add.cpp#L49-L56: Runclang-format -i examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add.cpp.examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add_scalar.cpp#L51-L59: Runclang-format -i examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add_scalar.cpp.examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_mul.cpp#L49-L56: Runclang-format -i examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_mul.cpp.examples/workers/l4/vector_add_mixed_l3/kernels/orchestration/vector_add_mixed_l3_orchestration.cpp#L42-L54: Runclang-format -i examples/workers/l4/vector_add_mixed_l3/kernels/orchestration/vector_add_mixed_l3_orchestration.cpp.
As per coding guidelines, **/*.{cc,cpp,cxx,h,hpp} requires clang-format -i <file>.
📍 Affects 4 files
examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add.cpp#L49-L56(this comment)examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add_scalar.cpp#L51-L59examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_mul.cpp#L49-L56examples/workers/l4/vector_add_mixed_l3/kernels/orchestration/vector_add_mixed_l3_orchestration.cpp#L42-L54
🤖 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 `@examples/workers/l4/vector_add_mixed_l3/kernels/aiv/kernel_add.cpp` around
lines 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).
Source: Coding guidelines
| try: | ||
| buf = inner_worker.create_host_buffer(int(nbytes)) | ||
| entry = _RemoteBufferEntry( | ||
| buf, | ||
| int(nbytes), | ||
| generation, | ||
| RemoteAddressSpace.REMOTE_DEVICE, | ||
| owner=inner_worker, | ||
| ) | ||
| except _NoHostBufferChildrenError: | ||
| buf = shared_memory.SharedMemory(create=True, size=int(nbytes)) | ||
| entry = _RemoteBufferEntry(buf, int(nbytes), generation, RemoteAddressSpace.REMOTE_DEVICE) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve an exportable descriptor for worker-owned buffers.
inner_worker.create_host_buffer() returns a HostBuffer, but this branch stores it in _RemoteBufferEntry.data. EXPORT_BUFFER later calls _RemoteBufferEntry.shm_name, which raises for every non-SharedMemory value. The preferred allocation path therefore cannot export a buffer. Store the backing shared-memory name or host TCP descriptor with the entry, then use it in ExportBufferResult.
Also applies to: 789-790
🤖 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/remote_l3_session.py` around lines 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.
- Add pure-Python endpoint selectors, registry resolution, capability cache, and SingleOwner backend planning. - Wire Worker internal resolve/plan helpers with lazy registry epochs while keeping create_region and materialization out of W2. - Cover selector, registry, planner, and package-surface behavior with focused unit tests.
7b9df75 to
74c9abf
Compare
ccyywwen
changed the title
- Move Sequence to collections.abc for ruff's typing rules - Split long endpoint and test expressions so ruff E501 passes
Keep region resolution and backend planning on the same EndpointRegistry instance so a concurrent close cannot clear the memo between lookups.
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.
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