Add: support mpirun-launched multi-host L3 workers - #1623
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 PR adds Global CommDomain support across native backends, Python APIs, local and remote L3 workers, MPI groups, lifecycle management, copy operations, smoke tests, documentation, and A5 CI jobs. ChangesGlobal CommDomain implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant Worker
participant RemoteL3Session
participant CommRuntime
Orchestrator->>Worker: allocate_global_domain
Worker->>RemoteL3Session: prepare and exchange descriptors
RemoteL3Session->>CommRuntime: prepare/import domain
CommRuntime-->>RemoteL3Session: descriptor or device context
RemoteL3Session-->>Worker: commit domain
Worker-->>Orchestrator: GlobalCommDomainHandle
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (19)
docs/comm-domain.md (1)
51-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument or link the MPI-launched topology.
The Global CommDomain section only covers forked local workers and TCP-connected workers, calling out that the smoke cases do not use
mpirun. If staticmpi-launched L4/L3 groups are supported, add the manifest, readiness, and rank-order contract here or link to an authoritative L4/L3 startup section.🤖 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/comm-domain.md` around lines 51 - 55, Update the “Global CommDomain across local and remote L3 nodes” section to document the supported MPI-launched L4/L3 topology, including its manifest, readiness requirements, and rank-order contract; if those details are defined elsewhere, link to the authoritative startup section instead. Keep the existing local fork and TCP-connected worker coverage intact.python/simpler/worker.py (2)
1924-1930: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the failed release in the L2 sweep.
_sweep_l2_global_domainsruns at chip-child teardown and drops every release error silently. A backend release failure there means a leaked HCCL window, and no diagnostic reaches the parent. Write the first failure to stderr so post-mortems can see it. This also addresses thetry-except-passstatic-analysis hint.♻️ Proposed refactor
def _sweep_l2_global_domains(cw: ChipWorker, store: _L2GlobalDomainStore) -> None: for domain_id in list(store.domains): store.domains.pop(domain_id, None) try: cw._impl.comm_global_domain_release(int(domain_id)) - except Exception: # noqa: BLE001 - pass + except Exception as exc: # noqa: BLE001 + sys.stderr.write( + f"_sweep_l2_global_domains: domain_id={domain_id} release failed: {type(exc).__name__}: {exc}\n" + ) + sys.stderr.flush()🤖 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 1924 - 1930, Update _sweep_l2_global_domains to report the first exception from comm_global_domain_release to stderr while continuing the sweep for remaining domains; replace the silent except/pass handling with a concise diagnostic that includes the failure details.Source: Linters/SAST tools
3683-3701: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake
_close_mpirun_groupstolerant of one failing group.Two problems exist in this loop:
- If the final
proc.wait(timeout=timeout_s)afterproc.kill()raisesTimeoutExpired, the exception leaves the loop. Later groups keep theirprocessandready_dir, and the temporary ready directories leak.- On the rollback path the first call is
proc.wait(timeout=timeout_s). A hungmpiruntherefore consumes the whole grace budget per group beforeterminate()runs.Collect per-group errors and always remove the ready directory.
♻️ Proposed refactor
def _close_mpirun_groups(self, *, timeout_s: float = _ROLLBACK_GRACEFUL_TIMEOUT_S) -> None: + errors: list[BaseException] = [] for group in reversed(self._mpi_l3_groups): proc = group.process if proc is not None: try: proc.wait(timeout=timeout_s) except subprocess.TimeoutExpired: proc.terminate() try: proc.wait(timeout=timeout_s) except subprocess.TimeoutExpired: proc.kill() - proc.wait(timeout=timeout_s) + try: + proc.wait(timeout=timeout_s) + except BaseException as exc: # noqa: BLE001 + errors.append(exc) finally: group.process = None if group.ready_dir is not None: shutil.rmtree(group.ready_dir, ignore_errors=True) group.ready_dir = None group.manifest_path = None + if errors: + raise errors[0]🤖 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 3683 - 3701, Update _close_mpirun_groups to isolate failures per group: ensure every cleanup attempt, including the wait after proc.kill(), is caught and recorded rather than escaping the loop, then continue processing remaining groups. On the rollback path, avoid spending the full timeout before terminate() by using the intended immediate or bounded termination sequence, while always clearing group.process and removing ready_dir/manifest_path in finally-style cleanup.src/common/hierarchical/remote_endpoint.cpp (1)
937-941: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove
result_bytesout of the reply.
run_controlreturns aControlReplyPayloadby value. Accessing.result_byteson that temporary copies the vector.COPY_FROM_DOMAINresults reachGLOBAL_DOMAIN_MAX_COPY_BYTES(8 MiB), so this adds one full buffer copy per domain read. Move the member out of the temporary instead.⚡ Proposed fix
std::vector<uint8_t> RemoteL3Endpoint::control_remote_domain( remote_l3::ControlName control_name, const std::vector<uint8_t> &command_bytes ) { - return run_control(control_name, command_bytes).result_bytes; + return std::move(run_control(control_name, command_bytes).result_bytes); }Run
clang-format -i src/common/hierarchical/remote_endpoint.cppafter the change.As per coding guidelines: "Run
clang-format -i <file>when formatting C++ code."🤖 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/hierarchical/remote_endpoint.cpp` around lines 937 - 941, Update RemoteL3Endpoint::control_remote_domain to move result_bytes from the temporary ControlReplyPayload returned by run_control instead of copying it. Preserve the existing return type and behavior, then run clang-format on the file.Source: Coding guidelines
python/simpler/global_comm_domain.py (1)
371-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
decode_comm_init_resultto one construction.The function builds
GlobalCommInitResulttwice only to order the reads. Read the fields into locals first, then construct once. The read order stays identical.♻️ Proposed refactor
def decode_comm_init_result(data: bytes) -> GlobalCommInitResult: reader = _Reader(data) - result = GlobalCommInitResult( - profile="", - max_ranks=reader.u32(), - descriptor_bytes=reader.u32(), - local_device_count=reader.u32(), - ) - result = GlobalCommInitResult( - profile=reader.string("profile"), - max_ranks=result.max_ranks, - descriptor_bytes=result.descriptor_bytes, - local_device_count=result.local_device_count, - ) + max_ranks = reader.u32() + descriptor_bytes = reader.u32() + local_device_count = reader.u32() + profile = reader.string("profile") reader.done("COMM_INIT result") - return result + return GlobalCommInitResult( + profile=profile, + max_ranks=max_ranks, + descriptor_bytes=descriptor_bytes, + local_device_count=local_device_count, + )🤖 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/global_comm_domain.py` around lines 371 - 386, Update decode_comm_init_result to read profile, max_ranks, descriptor_bytes, and local_device_count into local variables in the existing wire order, then construct GlobalCommInitResult exactly once with those values; keep reader.done("COMM_INIT result") and the returned result unchanged.python/simpler/task_interface.py (1)
1096-1106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a
__repr__for parity withCommDomainHandle.
CommDomainHandle.__repr__reports the live/released/freed state, which helps when a close() residual is logged.GlobalCommDomainHandlehas no__repr__, soWorker._describe_live_resourcesdiagnostics and stderr messages show the default object repr for these handles.♻️ Proposed addition
def __exit__(self, *_): self.release() + + def __repr__(self) -> str: + if self._freed: + state = "freed" + elif self._released: + state = "released-pending-free" + else: + state = "live" + return ( + f"GlobalCommDomainHandle(name={self.name!r}, domain_id={self.domain_id}, " + f"ranks={len(self.members)}, {state})" + )🤖 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/task_interface.py` around lines 1096 - 1106, Add a __repr__ method to GlobalCommDomainHandle matching CommDomainHandle’s representation, including the handle’s live, released, and freed state. Ensure Worker._describe_live_resources and stderr diagnostics display useful state information instead of the default object representation.python/simpler/remote_l3_worker.py (1)
67-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the accepted profiles from
GLOBAL_DOMAIN_PROFILE_IDS.The tuple
("sim", "a3-fabric-v1")and thea3-fabric-v1platform rule are repeated here and inpython/simpler/worker.py(RemoteWorkerSpec.__post_init__,MpiL3GroupSpec.__post_init__, andWorker._validate_global_node_config). A new profile must then be added in four places, and one missed copy makes the daemon reject a manifest the parent considers valid. Import the profile table and check membership against it.♻️ Proposed refactor
- comm_profile = str(manifest.get("comm_profile", manifest["transport"])) - if comm_profile not in ("sim", "a3-fabric-v1"): + comm_profile = str(manifest.get("comm_profile", manifest["transport"])) + if comm_profile not in GLOBAL_DOMAIN_PROFILE_IDS: raise ValueError("manifest comm_profile is not supported") - if comm_profile == "a3-fabric-v1" and not str(manifest["platform"]).startswith("a2a3"): + if comm_profile == GLOBAL_DOMAIN_PROFILE_A3_FABRIC and not str(manifest["platform"]).startswith("a2a3"): raise ValueError("manifest a3-fabric-v1 comm_profile requires an a2a3 platform") - if comm_profile == "a3-fabric-v1" and str(manifest["platform"]).endswith("sim"): + if comm_profile == GLOBAL_DOMAIN_PROFILE_A3_FABRIC and str(manifest["platform"]).endswith("sim"): raise ValueError("manifest a3-fabric-v1 comm_profile requires real A3 devices")Add the import at the top of the module:
from .global_comm_domain import GLOBAL_DOMAIN_PROFILE_A3_FABRIC, GLOBAL_DOMAIN_PROFILE_IDS🤖 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_worker.py` around lines 67 - 73, Update manifest validation around comm_profile to import and use GLOBAL_DOMAIN_PROFILE_IDS from global_comm_domain instead of the hard-coded profile tuple, while preserving the existing a3-fabric-v1 platform constraints via the shared GLOBAL_DOMAIN_PROFILE_A3_FABRIC definition.tools/remote_l4_npu/start_machine_daemon.sh (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
SIMPLER_REMOTE_L4_NPU_ROLEis only printed, never used.The script defaults the variable, echoes it, and then never passes it to
python -m simpler.remote_l3_worker. An operator who setsSIMPLER_REMOTE_L4_NPU_ROLEgets no behavior change. Either forward it to the daemon or drop the variable and the echo.Also applies to: 21-26
🤖 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 `@tools/remote_l4_npu/start_machine_daemon.sh` at line 14, Update start_machine_daemon.sh so SIMPLER_REMOTE_L4_NPU_ROLE affects the python -m simpler.remote_l3_worker invocation by forwarding the configured role through the daemon’s supported argument or environment mechanism; otherwise remove the unused default and echo. Ensure an operator-provided role changes daemon behavior and retain only relevant logging.python/simpler/orchestrator.py (1)
449-450: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd docstrings to the new public copy and release APIs.
allocate_global_domainandget_global_domaindocument their contract.release_global_domain,copy_to_global_domain, andcopy_from_global_domaindo not. These three methods carry non-obvious semantics that a caller cannot infer from the signature:domain_rankselects the target rank inside the domain,bufferselects a named carve-out, andoffsetis relative to that buffer whenbufferis given and relative to the mapped window otherwise.Document that offset semantics on each method.
Also applies to: 473-500
🤖 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/orchestrator.py` around lines 449 - 450, てsrc/common/hierarchical/worker.h (1)
183-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider naming this forwarder
control_remote_domain.Every neighboring forwarder uses the
control_*prefix and keeps the manager's name (control_prepare,control_alloc_domain,control_payload). This method inverts the words while forwarding tomanager_.control_remote_domain. Aligning the name keeps the control surface consistent. Update the Python binding site together with the rename.🤖 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/hierarchical/worker.h` around lines 183 - 187, Rename the worker forwarder method from remote_domain_control to control_remote_domain, preserving its delegation to manager_.control_remote_domain. Update the corresponding Python binding site and any references to use the renamed method consistently.src/common/platform_comm/comm_sim.cpp (1)
810-838: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider unmapping the peer mappings that this import already created when it fails.
A mid-loop
shm_openormmapfailure returns -1 withallocation->peer_mappingspartly filled andhost_ctxstill null.GlobalDomainAllocationcleans these up at release, so no mapping leaks permanently. However, a caller that retriescomm_global_domain_importfor the samedomain_idpasses thehost_ctx != nullptrgate again and appends a second set of peer mappings for the same windows, so the process holds duplicate mappings until release. Rolling back the mappings added by the failed attempt keeps the retry path allocation-neutral.♻️ Proposed rollback on the import failure path
auto ctx = std::make_unique<CommContext>(); ctx->rankId = allocation->rank; ctx->rankNum = allocation->nranks; ctx->winSize = allocation->mapping_size; + const size_t mappings_before = allocation->peer_mappings.size(); + auto rollback = [&]() { + for (size_t i = mappings_before; i < allocation->peer_mappings.size(); ++i) { + munmap(allocation->peer_mappings[i].base, allocation->peer_mappings[i].size); + } + allocation->peer_mappings.resize(mappings_before); + }; allocation->peer_mappings.reserve(allocation->nranks - 1); for (uint32_t rank = 0; rank < allocation->nranks; ++rank) { const auto *descriptor = rank_order[rank]; if (descriptor == nullptr) { + rollback(); return -1; } void *base = allocation->local_base; if (rank != allocation->rank) { std::string peer_name( reinterpret_cast<const char *>(descriptor->handle), static_cast<size_t>(descriptor->handle_size) ); int fd = shm_open(peer_name.c_str(), O_RDWR, 0600); if (fd < 0) { + rollback(); return -1; } base = mmap(nullptr, allocation->mapping_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); close(fd); if (base == MAP_FAILED) { + rollback(); return -1; }🤖 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_sim.cpp` around lines 810 - 838, On every failure path inside the peer-mapping loop in comm_global_domain_import, unmap and remove the GlobalPeerMapping entries created during the current import attempt before returning -1. Cover descriptor validation, shm_open failure, and mmap failure, preserving pre-existing mappings so retries do not append duplicates.tools/a3_l4_tcp_smoke/global_tload_smoke.py (1)
143-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRelease the global domain in a
finallyblock in both read callbacks. Both read-and-release callbacks calldomain.release()after the read loop with notry/finally. Ifcopy_from_global_domainraises, the release does not run.Worker.close()still tears the domain down, so no leak occurs, but the two scripts diverge from theverify_phasepattern intools/a3_l4_tcp_smoke/compute_then_tload_smoke.pyat lines 237-248.
tools/a3_l4_tcp_smoke/global_tload_smoke.py#L143-L152: wrap thefor rank in range(len(node_ids))read loop intryand movedomain.release()into afinallyblock.tools/a3_l4_tcp_smoke/mixed_global_tload_smoke.py#L164-L173: wrap thefor rank in range(node_count)read loop intryand movedomain.release()into afinallyblock.🤖 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 `@tools/a3_l4_tcp_smoke/global_tload_smoke.py` around lines 143 - 152, Ensure both read callbacks release their global domains in finally blocks: in tools/a3_l4_tcp_smoke/global_tload_smoke.py lines 143-152, wrap the rank read loop in read_and_release with try/finally and move domain.release() into finally; apply the same change to tools/a3_l4_tcp_smoke/mixed_global_tload_smoke.py lines 164-173 around its range(node_count) loop. Preserve the existing read and append behavior.tools/remote_l4_npu/remote_l4_npu_smoke.py (2)
273-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the ignored
remote_freefailures.The bare
except Exception: passhides every teardown error. Print the failure so an operator can see a leaked remote buffer. Cleanup still continues for the remaining handles.♻️ Proposed change
for handle in reversed(remote_buffers): try: worker.remote_free(handle) - except Exception: # noqa: BLE001 - pass + except Exception as error: # noqa: BLE001 + print(f"[remote-l4-group] remote_free failed: {error}")🤖 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 `@tools/remote_l4_npu/remote_l4_npu_smoke.py` around lines 273 - 278, Update the remote buffer cleanup loop around worker.remote_free to catch each exception and log the failure, including the affected handle and error details, while continuing to process remaining handles; preserve the subsequent worker.close() call.Source: Linters/SAST tools
41-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe shared keepalive slot drops references on a second invocation.
_REMOTE_GROUP_KEEPALIVE[:] = [...]replaces the previous contents. The comment states the inner Worker drains after the callback returns, so serial invocation is safe. If the remote L3 ever runs a secondremote_l3_group_orchtask before the first drains, the slice assignment releases the firstTaskArgswhile the native side may still read it. That failure is a use-after-free, and it is silent. Append instead of replacing, and clear the list only after the drain completes.🛡️ Proposed change
- _REMOTE_GROUP_KEEPALIVE[:] = [chip_args0, chip_args1] + _REMOTE_GROUP_KEEPALIVE.extend((chip_args0, chip_args1)) orch.submit_next_level_group(chip_handle, [chip_args0, chip_args1], cfg, workers=[0, 1])Also applies to: 66-69
🤖 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 `@tools/remote_l4_npu/remote_l4_npu_smoke.py` at line 41, Update the keepalive handling around _REMOTE_GROUP_KEEPALIVE and remote_l3_group_orch so each invocation appends its TaskArgs reference instead of replacing existing entries. Retain all references while native workers may still read them, and clear the shared list only after the inner Worker callback has completed and the drain is guaranteed.tests/ut/py/test_global_comm_domain.py (2)
744-745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider capturing daemon output for failure diagnosis.
Both tests discard daemon stdout and stderr. If a daemon fails to start,
_wait_for_tcp_portsraisesTimeoutErrorwith no cause. Capture the streams to a pipe or temporary file and print them when startup times out.Also applies to: 822-823
🤖 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 744 - 745, Update both daemon startup test paths around the subprocess stdout/stderr configuration to capture output instead of discarding it, then include the collected stdout and stderr in the diagnostic output when _wait_for_tcp_ports raises TimeoutError. Preserve normal startup behavior and ensure the captured streams are available for both referenced test cases.
665-666: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate
buffer_ptrsasClassVarto satisfy RUF012.Ruff flags the mutable dict class attribute. Add a
ClassVarannotation to keep the lint step clean.♻️ Proposed change
class FakeContext: - buffer_ptrs = {"lhs": 0x1000, "rhs": 0x2000, "input": 0x3000} + buffer_ptrs: ClassVar[dict[str, int]] = {"lhs": 0x1000, "rhs": 0x2000, "input": 0x3000}Add the import at the top of the file:
+from typing import ClassVar🤖 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 665 - 666, Update FakeContext.buffer_ptrs by annotating the mutable class-level dictionary as ClassVar, and add the corresponding typing import at the file level so RUF012 passes without changing its values or behavior.Source: Linters/SAST tools
tools/a3_l4_tcp_smoke/mpirun_compute_then_tload_2x2_smoke.py (1)
117-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument this smoke in the tool README.
tools/a3_l4_tcp_smoke/README.mddescribesglobal_tload_smoke.py,mixed_global_tload_smoke.py, andcompute_then_tload_smoke.py. It does not describe this mpirun 2x2 script. Add a section with the required arguments and the two-host prerequisites.Do you want me to draft the README section?
🤖 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 `@tools/a3_l4_tcp_smoke/mpirun_compute_then_tload_2x2_smoke.py` around lines 117 - 121, Update tools/a3_l4_tcp_smoke/README.md to document mpirun_compute_then_tload_2x2_smoke.py, including its required command-line arguments and the prerequisites for running across two hosts. Follow the structure and terminology of the existing smoke sections and describe the two-host setup requirements.tools/a3_l4_tcp_smoke/compute_then_tload_smoke.py (2)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse iterable unpacking instead of list concatenation.
Ruff reports RUF005 here. The same pattern appears in
tools/a3_l4_tcp_smoke/global_tload_smoke.pyandtools/remote_l4_npu/remote_l4_npu_smoke.py.♻️ Proposed change
- kernel_include_dirs = list(include_dirs) + [str(compiler.project_root / "src" / "common")] + kernel_include_dirs = [*include_dirs, str(compiler.project_root / "src" / "common")]🤖 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 `@tools/a3_l4_tcp_smoke/compute_then_tload_smoke.py` at line 46, Update the include-directory assignments in compute_then_tload_smoke.py, global_tload_smoke.py, and remote_l4_npu_smoke.py to use iterable unpacking instead of list concatenation, preserving the existing include_dirs entries and appended common source directory.Source: Linters/SAST tools
41-41: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueIncrease the global domain window to avoid tight carving.
WINDOW_SIZEis 4096 while the fourCommBufferSpecentries each request 1024 bytes, so the total is exactly 4096. Buffer carving is sequential, so this leaves no slack for any future alignment/header/metadata padding; add a small extra window margin before the next non-zero buffer and any future allocations.🤖 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 `@tools/a3_l4_tcp_smoke/compute_then_tload_smoke.py` at line 41, Increase the global WINDOW_SIZE constant above the current 4096-byte total so the four 1024-byte CommBufferSpec allocations have slack for alignment, metadata, and future buffers. Preserve the existing buffer definitions and carving behavior.
🤖 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/comm-domain.md`:
- Around line 90-92: Update the documentation sentence around
GlobalCommDomainHandle.buffer_range() and _global_copy_range() to distinguish
their bounds sources: named-buffer offset and limit calculations use each
buffer’s nbytes, while unbuffered mapped-window validation uses the returned
mapping_size. Keep the existing statements about backend-reported mapped size
and A3 Fabric alignment intact.
- Around line 92-95: Resolve the contradiction between the ChipDomainContext API
fields and the statement that device pointers never cross the public Python API:
update the wording near orch.get_global_domain(domain_id) to limit the
no-pointer claim specifically to the L4 GlobalCommDomainHandle, or revise the
earlier ChipDomainContext API table so both sections describe the same
public-pointer contract.
In `@docs/remote-l3-worker-design/protocol.md`:
- Around line 315-318: Clarify the RELEASE_DOMAIN description by distinguishing
the release request from physical deallocation: state when the handle is marked
released, that backend teardown is deferred until the run fence, and how this
applies to retain_after_run allocations, explicit release, and session shutdown.
Align the wording with the release() semantics documented in comm-domain.md.
In `@python/bindings/worker_bind.h`:
- Around line 706-716: Update the worker control binding lambda before
constructing command_bytes or calling remote_domain_control to validate
control_name with remote_l3::valid_control_name(control_name). Reject invalid
values at the binding boundary, and only perform the static_cast to
remote_l3::ControlName after validation.
In `@python/simpler/mpi_l3_session.py`:
- Around line 48-118: Update Worker._global_domain_control_many and
prepare_import so fan-out failures or missing ranks trigger a bounded timeout
and cancellation/cleanup of outstanding ALLOC_DOMAIN work and MPI collective
progress. Ensure a failed allgather cannot leave other ranks blocked
indefinitely, and unwind prepared global-domain state before propagating the
failure to _close_mpirun_groups.
In `@python/simpler/remote_l3_session.py`:
- Around line 698-714: Update the REMOTE_DEVICE allocation path in
_RemoteBufferEntry handling so EXPORT_BUFFER can safely process
HostBuffer-backed entries. Resolve the backing shared-memory name for HostBuffer
instances before the entry.shm_name.encode path, or explicitly reject the
allocation as non-exportable before reaching it; preserve direct SharedMemory
export behavior.
In `@python/simpler/worker.py`:
- Around line 564-570: Update the zip call in the validation loop over
global_device_ranks_by_rank and device_ids_by_rank to pass strict=True,
preserving the existing iteration and validation behavior while resolving Ruff
B905.
- Around line 1764-1790: Wrap the GlobalDomainDescriptor.decode call in the
global-domain preparation flow with cleanup that invokes
cw._impl.comm_global_domain_release(int(domain_id)) when decoding raises,
matching the existing inconsistent-descriptor release path. Ensure the exception
still propagates and that successful decoding continues into the descriptor
validation and store.domains insertion unchanged.
In `@src/a2a3/platform/onboard/host/comm_hccl.cpp`:
- Around line 1636-1660: Update the import failure path in the peer-window loop
to release and clear all previously imported entries in allocation->peer_windows
before returning -1. Ensure retries through _handle_ctrl_global_domain_import do
not retain or append duplicate mappings, while preserving the existing error
logging and successful import behavior.
In `@tests/ut/py/test_global_comm_domain.py`:
- Around line 704-707: Replace the single-port helper _free_tcp_port with a
helper that reserves and returns the requested number of distinct ephemeral
ports while keeping all sockets open until every port is selected; update the
call sites to request two ports together where both daemons are configured and
one port for the single-port case, preserving existing port usage.
In `@tools/a3_l4_tcp_smoke/kernels/aiv/global_tload_kernel.cpp`:
- Around line 22-34: Update the attribute on CommRemotePtr to use the already
guarded __aicore__ spelling, or define AICORE as a fallback alongside the
existing __aicore__ guard before the function. Ensure CommRemotePtr and
kernel_entry use a consistently available accelerator-core attribute without
changing the function’s behavior.
In `@tools/a3_l4_tcp_smoke/mpirun_compute_then_tload_2x2_smoke.py`:
- Around line 37-41: Update _parse_csv_ints to reject any negative device ID and
reject duplicate IDs, raising a clear ValueError before returning the parsed
tuple; preserve acceptance of non-empty, unique non-negative integers and apply
the same validation wherever this helper is used.
- Line 10: Replace the specific lab machine names "35/37" in the module
docstring with a generic description of the two-host layout. Remove the
hardcoded default values for host and RoCE IP addresses in the argument parser
setup (around lines 214-219), make those arguments required instead, and update
all attribute name references in the run function from specific machine
identifiers (args.host_37, args.host_35) to generic placeholders (args.host_a,
args.host_b). This removes private network topology details from the code while
maintaining the two-host computation pattern as the core functionality.
In `@tools/a3_l4_tcp_smoke/README.md`:
- Line 7: Update the README description to use the fabric profile name
“a3-fabric-v1” instead of “Fabric V2,” matching the comm_profile requested by
the smoke scripts.
In `@tools/remote_l4_npu/remote_l4_npu_smoke.py`:
- Line 177: Update the argument definition for --session-listen-host to require
an explicitly supplied host instead of defaulting to 0.0.0.0. In the Worker
construction calls around the session listener setup, derive
allow_wildcard_session_bind from whether the supplied host is a wildcard
address, enabling it only for an explicitly requested wildcard and preserving it
as disabled for specific reachable addresses.
In `@tools/remote_l4_npu/start_machine_daemon.sh`:
- Line 15: Change the default value of SIMPLER_REMOTE_L4_NPU_HOST in
start_machine_daemon.sh from 0.0.0.0 to 127.0.0.1, preserving explicit
environment overrides for users who intentionally need a routable bind address.
---
Nitpick comments:
In `@docs/comm-domain.md`:
- Around line 51-55: Update the “Global CommDomain across local and remote L3
nodes” section to document the supported MPI-launched L4/L3 topology, including
its manifest, readiness requirements, and rank-order contract; if those details
are defined elsewhere, link to the authoritative startup section instead. Keep
the existing local fork and TCP-connected worker coverage intact.
In `@python/simpler/global_comm_domain.py`:
- Around line 371-386: Update decode_comm_init_result to read profile,
max_ranks, descriptor_bytes, and local_device_count into local variables in the
existing wire order, then construct GlobalCommInitResult exactly once with those
values; keep reader.done("COMM_INIT result") and the returned result unchanged.
In `@python/simpler/orchestrator.py`:
- Around line 449-450: て
In `@python/simpler/remote_l3_worker.py`:
- Around line 67-73: Update manifest validation around comm_profile to import
and use GLOBAL_DOMAIN_PROFILE_IDS from global_comm_domain instead of the
hard-coded profile tuple, while preserving the existing a3-fabric-v1 platform
constraints via the shared GLOBAL_DOMAIN_PROFILE_A3_FABRIC definition.
In `@python/simpler/task_interface.py`:
- Around line 1096-1106: Add a __repr__ method to GlobalCommDomainHandle
matching CommDomainHandle’s representation, including the handle’s live,
released, and freed state. Ensure Worker._describe_live_resources and stderr
diagnostics display useful state information instead of the default object
representation.
In `@python/simpler/worker.py`:
- Around line 1924-1930: Update _sweep_l2_global_domains to report the first
exception from comm_global_domain_release to stderr while continuing the sweep
for remaining domains; replace the silent except/pass handling with a concise
diagnostic that includes the failure details.
- Around line 3683-3701: Update _close_mpirun_groups to isolate failures per
group: ensure every cleanup attempt, including the wait after proc.kill(), is
caught and recorded rather than escaping the loop, then continue processing
remaining groups. On the rollback path, avoid spending the full timeout before
terminate() by using the intended immediate or bounded termination sequence,
while always clearing group.process and removing ready_dir/manifest_path in
finally-style cleanup.
In `@src/common/hierarchical/remote_endpoint.cpp`:
- Around line 937-941: Update RemoteL3Endpoint::control_remote_domain to move
result_bytes from the temporary ControlReplyPayload returned by run_control
instead of copying it. Preserve the existing return type and behavior, then run
clang-format on the file.
In `@src/common/hierarchical/worker.h`:
- Around line 183-187: Rename the worker forwarder method from
remote_domain_control to control_remote_domain, preserving its delegation to
manager_.control_remote_domain. Update the corresponding Python binding site and
any references to use the renamed method consistently.
In `@src/common/platform_comm/comm_sim.cpp`:
- Around line 810-838: On every failure path inside the peer-mapping loop in
comm_global_domain_import, unmap and remove the GlobalPeerMapping entries
created during the current import attempt before returning -1. Cover descriptor
validation, shm_open failure, and mmap failure, preserving pre-existing mappings
so retries do not append duplicates.
In `@tests/ut/py/test_global_comm_domain.py`:
- Around line 744-745: Update both daemon startup test paths around the
subprocess stdout/stderr configuration to capture output instead of discarding
it, then include the collected stdout and stderr in the diagnostic output when
_wait_for_tcp_ports raises TimeoutError. Preserve normal startup behavior and
ensure the captured streams are available for both referenced test cases.
- Around line 665-666: Update FakeContext.buffer_ptrs by annotating the mutable
class-level dictionary as ClassVar, and add the corresponding typing import at
the file level so RUF012 passes without changing its values or behavior.
In `@tools/a3_l4_tcp_smoke/compute_then_tload_smoke.py`:
- Line 46: Update the include-directory assignments in
compute_then_tload_smoke.py, global_tload_smoke.py, and remote_l4_npu_smoke.py
to use iterable unpacking instead of list concatenation, preserving the existing
include_dirs entries and appended common source directory.
- Line 41: Increase the global WINDOW_SIZE constant above the current 4096-byte
total so the four 1024-byte CommBufferSpec allocations have slack for alignment,
metadata, and future buffers. Preserve the existing buffer definitions and
carving behavior.
In `@tools/a3_l4_tcp_smoke/global_tload_smoke.py`:
- Around line 143-152: Ensure both read callbacks release their global domains
in finally blocks: in tools/a3_l4_tcp_smoke/global_tload_smoke.py lines 143-152,
wrap the rank read loop in read_and_release with try/finally and move
domain.release() into finally; apply the same change to
tools/a3_l4_tcp_smoke/mixed_global_tload_smoke.py lines 164-173 around its
range(node_count) loop. Preserve the existing read and append behavior.
In `@tools/a3_l4_tcp_smoke/mpirun_compute_then_tload_2x2_smoke.py`:
- Around line 117-121: Update tools/a3_l4_tcp_smoke/README.md to document
mpirun_compute_then_tload_2x2_smoke.py, including its required command-line
arguments and the prerequisites for running across two hosts. Follow the
structure and terminology of the existing smoke sections and describe the
two-host setup requirements.
In `@tools/remote_l4_npu/remote_l4_npu_smoke.py`:
- Around line 273-278: Update the remote buffer cleanup loop around
worker.remote_free to catch each exception and log the failure, including the
affected handle and error details, while continuing to process remaining
handles; preserve the subsequent worker.close() call.
- Line 41: Update the keepalive handling around _REMOTE_GROUP_KEEPALIVE and
remote_l3_group_orch so each invocation appends its TaskArgs reference instead
of replacing existing entries. Retain all references while native workers may
still read them, and clear the shared list only after the inner Worker callback
has completed and the drain is guaranteed.
In `@tools/remote_l4_npu/start_machine_daemon.sh`:
- Line 14: Update start_machine_daemon.sh so SIMPLER_REMOTE_L4_NPU_ROLE affects
the python -m simpler.remote_l3_worker invocation by forwarding the configured
role through the daemon’s supported argument or environment mechanism; otherwise
remove the unused default and echo. Ensure an operator-provided role changes
daemon behavior and retain only relevant logging.
🪄 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: 29a64aec-682b-40c2-90d0-c135fd63ab80
📒 Files selected for processing (46)
.github/workflows/ci.ymldocs/comm-domain.mddocs/remote-l3-worker-design.mddocs/remote-l3-worker-design/implementation-record.mddocs/remote-l3-worker-design/protocol.mdpython/bindings/CMakeLists.txtpython/bindings/task_interface.cpppython/bindings/worker_bind.hpython/simpler/global_comm_domain.pypython/simpler/global_comm_smoke.pypython/simpler/mpi_l3_session.pypython/simpler/orchestrator.pypython/simpler/remote_l3_protocol.pypython/simpler/remote_l3_session.pypython/simpler/remote_l3_worker.pypython/simpler/task_interface.pypython/simpler/worker.pysrc/a2a3/platform/onboard/host/comm_hccl.cppsrc/a5/platform/onboard/host/comm_hccl.cppsrc/common/hierarchical/remote_endpoint.cppsrc/common/hierarchical/remote_endpoint.hsrc/common/hierarchical/remote_wire.cppsrc/common/hierarchical/remote_wire.hsrc/common/hierarchical/worker.hsrc/common/hierarchical/worker_manager.cppsrc/common/hierarchical/worker_manager.hsrc/common/platform_comm/comm.hsrc/common/platform_comm/comm_sim.cppsrc/common/worker/chip_worker.cppsrc/common/worker/chip_worker.htests/ut/cpp/CMakeLists.txttests/ut/py/test_callable_identity.pytests/ut/py/test_global_comm_domain.pytools/a3_l4_tcp_smoke/README.mdtools/a3_l4_tcp_smoke/compute_then_tload_smoke.pytools/a3_l4_tcp_smoke/global_tload_smoke.pytools/a3_l4_tcp_smoke/kernels/aiv/global_tload_kernel.cpptools/a3_l4_tcp_smoke/kernels/aiv/local_add_kernel.cpptools/a3_l4_tcp_smoke/kernels/orchestration/global_tload_orch.cpptools/a3_l4_tcp_smoke/kernels/orchestration/local_add_orch.cpptools/a3_l4_tcp_smoke/mixed_global_tload_smoke.pytools/a3_l4_tcp_smoke/mpirun_compute_then_tload_2x2_smoke.pytools/remote_l4_npu/README.mdtools/remote_l4_npu/remote_l4_npu_smoke.pytools/remote_l4_npu/run_parent_smoke.shtools/remote_l4_npu/start_machine_daemon.sh
90cba14 to
48281af
Compare
|
@coderabbitai Addressed the review-body follow-ups in the single squashed commit:
The remaining review-body notes reference tool/test code that is no longer present in the current PR diff, so no code change was needed for those items. |
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 12 minutes. |
48281af to
aca0399
Compare
Add static MPI-launched L3 groups with rank-0 manifest broadcast and ready coordination, and extend Global CommDomains across mixed local, TCP-remote, and MPI-backed L3 nodes. Harden the control path with validated control names, bounded MPI collectives and L4 fanout, rollback-safe peer mappings, isolated process cleanup, fixed-port session binding compatibility, diagnostics, documentation, and regression coverage. Co-authored-by: Leaf-Salix <2503954024@qq.com> Co-authored-by: xl <21039015+xl1123@users.noreply.github.com>
aca0399 to
d3d17b2
Compare
1 participant
Dependency
c293351c(Add: support mixed local and remote L4 CommDomains)Summary
Testing
pytest tests/ut/py/test_global_comm_domain.py -m "not requires_hardware"on the validated implementation (25 passed)max_diff == 0)fcntlmodule