Skip to content

Add: support mixed local and remote L4 CommDomains - #1456

Open
sunkaixuan2018 wants to merge 1 commit into
hw-native-sys:mainfrom
sunkaixuan2018:two-server-L4-communication
Open

Add: support mixed local and remote L4 CommDomains#1456
sunkaixuan2018 wants to merge 1 commit into
hw-native-sys:mainfrom
sunkaixuan2018:two-server-L4-communication

Conversation

@sunkaixuan2018

@sunkaixuan2018 sunkaixuan2018 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add an L4-brokered Global CommDomain flow that collects rank-ordered L2 export descriptors through L3 and returns the complete table for L2 import, without mpirun.
  • Support one domain spanning both forked local L3 workers (add_worker) and TCP-connected remote L3 workers (add_remote_worker).
  • Report only backend capabilities that are actually implemented: sim platforms accept sim, real A2/A3 accepts a3-fabric-v1, and unsupported platform/profile pairs are rejected before PREPARE and again at L3 COMM_INIT.
  • Harden Global CommDomain lifetime handling: same-name replacements are identity-safe, partial A3 release failures remain terminally visible, and childless-buffer fallback uses a dedicated exception.
  • Add PREPARE/IMPORT/COMMIT/ABORT failure-injection coverage plus remote-only and mixed local/remote peer-TLOAD, compute-then-communication, and per-node two-NPU group-compute smokes.

Testing

  • Current head 34c8b742: removed the obsolete CallConfig.block_dim assignment found by the two-machine group-compute validation. Local runtime tests and builds were intentionally not run because this workstation has no target test environment; the group-compute smoke is pending a remote rerun.
  • Current head formatting/static checks passed: repository commit hooks (headers, English-only, Ruff, and Pyright), explicit Ruff check/format verification, and git diff --check.
  • Remote head d909364a: focused tests passed (104 passed), including capability and PREPARE/IMPORT/COMMIT/ABORT failure injection. Two-host Global TLOAD, compute-then-TLOAD, and mixed local/remote L3 peer-TLOAD passed with every reported max_diff=0 and no mpirun.
  • On d909364a, the group-compute smoke stopped before remote dispatch because the removed CallConfig.block_dim field was still assigned by the smoke. Current head deletes that single stale assignment; no runtime implementation changed.
  • No internal logs or machine details are uploaded in this PR.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This change adds Global CommDomain support across remote L3 workers, including wire codecs, transactional allocation and cleanup, simulation and A3 backend implementations, Python APIs, remote control handling, tests, and end-to-end smoke workflows.

Changes

Global CommDomain

Layer / File(s) Summary
Protocol contracts and communication backends
python/simpler/global_comm_domain.py, src/common/platform_comm/*, src/a2a3/.../comm_hccl.cpp, src/a5/.../comm_hccl.cpp
Defines validated domain messages and descriptors, then implements prepare/import/release for simulation and A3 Fabric backends; A5 exports unsupported stubs.
Control transport and runtime bindings
src/common/hierarchical/*, src/common/worker/*, python/bindings/*
Routes domain controls through workers and remote endpoints, exposes byte-oriented Python bindings, and dynamically loads the new communicator APIs.
Remote session and L2 control handling
python/simpler/remote_l3_session.py, python/simpler/remote_l3_worker.py, python/simpler/worker.py
Validates node manifests, handles COMM_INIT and domain phases, performs copy operations, manages host buffers, and sweeps child-side domain state.
Python domain API and transaction orchestration
python/simpler/orchestrator.py, python/simpler/task_interface.py, python/simpler/worker.py
Adds domain handles, committed views, allocation/release/copy APIs, remote manifest fields, transactional commit/abort logic, and run/close cleanup.
Validation, smoke workflows, and documentation
tests/ut/py/*, tools/a3_l4_tcp_smoke/*, tools/remote_l4_npu/*, docs/*
Adds codec and integration tests, A3 compute/TLOAD kernels and drivers, remote NPU smoke scripts, and protocol/design documentation.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Poem

I’m a rabbit with domains in a row,
Descriptors hop where the remote winds blow.
Prepare, import, commit with delight,
Then TLOAD makes the tensors unite.
Copy and release, cleanup so neat—
A carrot-powered control-plane feat!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.66% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately reflects the main change: mixed local and remote L4 CommDomains.
Description check ✅ Passed The description is clearly related to the changes and summarizes the new Global CommDomain workflow and tests.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sunkaixuan2018 sunkaixuan2018 changed the title Add: support L4-brokered cross-node CommDomains Add: support L4-brokered cross-node CommDomains(without mpirun) Jul 24, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
tests/ut/py/test_global_comm_domain.py (1)

176-189: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Fixed time.sleep(0.3) daemon warmup is racy. worker.init() connects to the freshly spawned daemons, but there is no guarantee they have bound their ports within 300 ms on a loaded CI host, which can cause intermittent connection failures. Consider polling each (127.0.0.1, port) for accept-readiness (short connect-retry loop with a bounded deadline) before add_remote_worker/init instead of a static sleep.

🤖 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_global_comm_domain.py` around lines 176 - 189, Replace the
fixed time.sleep(0.3) in the worker setup test with bounded polling that
attempts short TCP connections to each 127.0.0.1 port until all daemons accept
connections or a deadline is reached. Perform this readiness check before the
add_remote_worker calls and worker.init(), and fail clearly if any daemon
remains unavailable.
src/common/platform_comm/comm.h (1)

48-57: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Pin the descriptor ABI used by the wire codec.

ChipWorker::comm_global_domain_prepare serializes sizeof(CommGlobalDomainDescriptor), while the Python wire contract fixes this descriptor at 288 bytes. Add size/offset static_asserts or explicit field serialization so compiler packing changes cannot silently break remote imports.

🤖 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 `@src/common/platform_comm/comm.h` around lines 48 - 57, Pin the
CommGlobalDomainDescriptor wire layout used by
ChipWorker::comm_global_domain_prepare: add compile-time size and field-offset
assertions matching the 288-byte Python contract, including the handle field, so
packing changes fail at build time rather than altering serialized imports.
🤖 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 `@docs/remote-l3-worker-design/implementation-record.md`:
- Around line 91-94: Update the L4-brokered Global CommDomain setup description
to state that domains are released after the L4 DAG drain by default, while
explicitly noting that retain_after_run=True preserves them for a later run.

In `@python/simpler/task_interface.py`:
- Around line 910-921: Sort the entries alphabetically in both __slots__ tuples
in the relevant classes, including the declaration containing name and members
and the second declaration around the referenced location, while preserving
every existing slot name.
- Around line 975-979: Update release so _released is set to True only after
_release_fn(self) completes successfully. If the remote cleanup raises,
propagate the error while leaving the handle unreleased so a later release()
retries the cleanup.

---

Nitpick comments:
In `@src/common/platform_comm/comm.h`:
- Around line 48-57: Pin the CommGlobalDomainDescriptor wire layout used by
ChipWorker::comm_global_domain_prepare: add compile-time size and field-offset
assertions matching the 288-byte Python contract, including the handle field, so
packing changes fail at build time rather than altering serialized imports.

In `@tests/ut/py/test_global_comm_domain.py`:
- Around line 176-189: Replace the fixed time.sleep(0.3) in the worker setup
test with bounded polling that attempts short TCP connections to each 127.0.0.1
port until all daemons accept connections or a deadline is reached. Perform this
readiness check before the add_remote_worker calls and worker.init(), and fail
clearly if any daemon remains unavailable.
🪄 Autofix (Beta)

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: f217e7f8-c970-4de9-9e33-aa5539b441f5

📥 Commits

Reviewing files that changed from the base of the PR and between d0bc661 and 73c4500.

📒 Files selected for processing (41)
  • docs/comm-domain.md
  • docs/remote-l3-worker-design.md
  • docs/remote-l3-worker-design/implementation-record.md
  • docs/remote-l3-worker-design/protocol.md
  • python/bindings/CMakeLists.txt
  • python/bindings/task_interface.cpp
  • python/bindings/worker_bind.h
  • python/simpler/global_comm_domain.py
  • python/simpler/global_comm_smoke.py
  • python/simpler/orchestrator.py
  • python/simpler/remote_l3_protocol.py
  • python/simpler/remote_l3_session.py
  • python/simpler/remote_l3_worker.py
  • python/simpler/task_interface.py
  • python/simpler/worker.py
  • src/a2a3/platform/onboard/host/comm_hccl.cpp
  • src/a5/platform/onboard/host/comm_hccl.cpp
  • src/common/hierarchical/remote_endpoint.cpp
  • src/common/hierarchical/remote_endpoint.h
  • src/common/hierarchical/remote_wire.cpp
  • src/common/hierarchical/remote_wire.h
  • src/common/hierarchical/worker.h
  • src/common/hierarchical/worker_manager.cpp
  • src/common/hierarchical/worker_manager.h
  • src/common/platform_comm/comm.h
  • src/common/platform_comm/comm_sim.cpp
  • src/common/worker/chip_worker.cpp
  • src/common/worker/chip_worker.h
  • tests/ut/py/test_callable_identity.py
  • tests/ut/py/test_global_comm_domain.py
  • tools/a3_l4_tcp_smoke/README.md
  • tools/a3_l4_tcp_smoke/compute_then_tload_smoke.py
  • tools/a3_l4_tcp_smoke/global_tload_smoke.py
  • tools/a3_l4_tcp_smoke/kernels/aiv/global_tload_kernel.cpp
  • tools/a3_l4_tcp_smoke/kernels/aiv/local_add_kernel.cpp
  • tools/a3_l4_tcp_smoke/kernels/orchestration/global_tload_orch.cpp
  • tools/a3_l4_tcp_smoke/kernels/orchestration/local_add_orch.cpp
  • tools/remote_l4_npu/README.md
  • tools/remote_l4_npu/remote_l4_npu_smoke.py
  • tools/remote_l4_npu/run_parent_smoke.sh
  • tools/remote_l4_npu/start_machine_daemon.sh

Comment thread docs/remote-l3-worker-design/implementation-record.md Outdated
Comment thread python/simpler/task_interface.py
Comment thread python/simpler/task_interface.py
@sunkaixuan2018
sunkaixuan2018 force-pushed the two-server-L4-communication branch 2 times, most recently from a49e5f4 to d7afa04 Compare July 24, 2026 08:40
@sunkaixuan2018

Copy link
Copy Markdown
Contributor Author

Also addressed the two CodeRabbit nitpicks listed in the review summary:

  • Replaced the fixed daemon warmup sleep with bounded TCP readiness polling.
  • Pinned the Global CommDomain descriptor to the 288-byte wire ABI with compile-time size and field-offset checks.

@sunkaixuan2018
sunkaixuan2018 force-pushed the two-server-L4-communication branch 7 times, most recently from e8bfbf8 to cd902af Compare July 24, 2026 14:08
@sunkaixuan2018
sunkaixuan2018 force-pushed the two-server-L4-communication branch 3 times, most recently from 3f0ba5a to 32f8fe6 Compare July 28, 2026 09:36
@sunkaixuan2018 sunkaixuan2018 changed the title Add: support L4-brokered cross-node CommDomains(without mpirun) Add: support mixed local and remote L4 CommDomains Jul 28, 2026
@sunkaixuan2018
sunkaixuan2018 force-pushed the two-server-L4-communication branch from 32f8fe6 to d909364 Compare July 28, 2026 10:58
- Relay rank-ordered Fabric descriptors through L4 to local and remote L3 nodes
- Route local Global CommDomain control through NEXT_LEVEL mailboxes
- Reject unsupported platform/profile capabilities before allocation and at COMM_INIT
- Make same-name release tracking identity-safe and release failures terminal
- Replace message matching with a dedicated childless-buffer exception
- Add transaction failure injection plus remote, mixed-node, compute, and communication smokes
- Document no-mpirun setup and retained Global CommDomain lifetimes

Co-authored-by: Leaf-Salix <2503954024@qq.com>
@sunkaixuan2018
sunkaixuan2018 force-pushed the two-server-L4-communication branch from d909364 to 34c8b74 Compare July 28, 2026 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant