Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a distributed communication framework for the A2A3 platform, supporting HCCL hardware and shared-memory simulation backends. It introduces a per-chip bootstrap mechanism for communicator initialization, device memory allocation, and data staging, supported by the new DistChipBootstrapChannel. The Worker class and CodeRunner utility were refactored to handle level 3 distributed execution and process lifecycle management, with corresponding extensions to the Python bindings and C API. Feedback identifies a breaking change in the task.orch function signature and suggests optimizing performance by moving a module import out of a loop in the bootstrap method. Comprehensive unit tests for the bootstrap channel and worker API are also included.
| self._orch._scope_begin() | ||
| try: | ||
| task.orch(self._orch, task.args) | ||
| task.orch(self, task.args) |
There was a problem hiding this comment.
The signature of the orchestrator function task.orch has been changed from orch(orchestrator, args) to orch(worker, args). This is a significant breaking change that should be documented in the pull request description. While the new API of calling methods on the worker instance is an improvement, the lack of documentation for this change can cause issues for users of this class.
| if len(blob) != size: | ||
| raise ValueError("input blob size must match buffer size") | ||
| if size > 0: | ||
| import ctypes as _ct |
There was a problem hiding this comment.
The import ctypes as _ct statement is inside a loop. While this works, it's inefficient to re-import the module on every iteration. For better performance and code style, please move this import to the top of the bootstrap method (e.g., on line 304).
- Restore latest main Worker forwarding methods dropped during conflict resolution - Apply clang-format to rebased binding and bootstrap C++ changes - Keep PR 571 bootstrap integration compatible with PR 592 HCCL base
docs/python-packaging.md rule 2 already bans sys.path.insert outside examples/scripts/build_runtimes.py, but four spots under tests/ut/py/ still carried copies: - tests/ut/py/conftest.py: inserted python/ and examples/scripts/ onto sys.path. Both are redundant — pip install makes simpler / simpler_setup / _task_interface importable via site-packages, and no test imports anything from examples/scripts/. File's only content was the sys.path hack, so it goes away entirely. - tests/ut/py/test_chip_worker.py: inserted python/ to find _task_interface. Already installed at the wheel root. - tests/ut/py/test_task_interface.py: same hack, same reason. - tests/ut/py/test_worker/test_platform_comm.py: inserted project root and python/. Inherited from the stash of PR hw-native-sys#571 which was written for standalone execution; PR hw-native-sys#597 (L1b) always relies on the installed package so the hack is noise. Ran `pytest tests/ut/py --ignore=tests/ut/py/test_hostsub_fork_shm.py` before and after the removal: 170 passed, 6 skipped in both cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
docs/python-packaging.md rule 2 bans sys.path.insert outside examples/scripts/build_runtimes.py. Three test files under tests/ut/py carried their own sys.path inserts that duplicated what tests/ut/py/conftest.py already does (and, under any install mode, they are covered by site-packages anyway): - tests/ut/py/test_chip_worker.py: inserted python/ to find _task_interface. Already on sys.path via the sibling conftest and installed at the wheel root under pip install. - tests/ut/py/test_task_interface.py: same hack, same reason. - tests/ut/py/test_worker/test_platform_comm.py: inserted project root and python/. Inherited from the stash of PR hw-native-sys#571 which was written for standalone execution; PR hw-native-sys#597 (L1b) always relies on the installed package or conftest-managed sys.path. tests/ut/py/conftest.py is deliberately left alone — upstream PR hw-native-sys#600 just refreshed its sys.path list for the no-install workflow (\"importable without installing the package\"), so the conftest-level hack is a maintained design choice; the per-test duplicates were simply redundant. Ran `pytest tests/ut/py --ignore=tests/ut/py/test_hostsub_fork_shm.py` before and after the removal: 170 passed, 6 skipped in both cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
docs/python-packaging.md rule 2 bans sys.path.insert outside examples/scripts/build_runtimes.py. Three test files under tests/ut/py carried their own sys.path inserts that duplicated what tests/ut/py/conftest.py already does (and, under any install mode, they are covered by site-packages anyway): - tests/ut/py/test_chip_worker.py: inserted python/ to find _task_interface. Already on sys.path via the sibling conftest and installed at the wheel root under pip install. - tests/ut/py/test_task_interface.py: same hack, same reason. - tests/ut/py/test_worker/test_platform_comm.py: inserted project root and python/. Inherited from the stash of PR #571 which was written for standalone execution; PR #597 (L1b) always relies on the installed package or conftest-managed sys.path. tests/ut/py/conftest.py is deliberately left alone — upstream PR #600 just refreshed its sys.path list for the no-install workflow (\"importable without installing the package\"), so the conftest-level hack is a maintained design choice; the per-test duplicates were simply redundant. Ran `pytest tests/ut/py --ignore=tests/ut/py/test_hostsub_fork_shm.py` before and after the removal: 170 passed, 6 skipped in both cases.
1 participant
No description provided.