feat(st): relocate collective examples into tests/st scene tests#1294
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR removes legacy L3 distributed collective example scripts/tests (allreduce_distributed, allreduce_ring_distributed, allgather_distributed, broadcast_distributed, reduce_scatter_distributed, all_to_all_distributed), rewrites the allreduce example as a minimal onephase implementation, adds shared collective test helpers, and introduces new scene test suites under tests/st/a2a3/tensormap_and_ringbuffer/collectives with corresponding docs. A kernel compiler include-path fix is also included. ChangesCollective examples consolidation
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Code Review
This pull request consolidates and moves several collective algorithm examples (allreduce, allgather, reduce_scatter, broadcast, and all_to_all) from examples/workers/l3/ to tests/st/a2a3/tensormap_and_ringbuffer/collectives/ as scene tests, while leaving a minimal allreduce example in the examples directory. Feedback on the new shared test helpers highlights a lack of validation for nranks and mode_id parameters in allreduce_orch_fn and generic_collective_orch_fn, which could lead to out-of-bounds memory allocation or access on the device.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1488f13 to
d4fbf83
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 153-154: The docs link in comm-domain.md points too far up the
directory tree and resolves outside the repo when clicked. Update the allreduce
tests link target used in the bullet so it stays relative to the docs directory
and lands on the intended allreduce scene tests, using the same link text but
the corrected path from the markdown entry.
In `@examples/workers/l3/allreduce/README.md`:
- Line 9: The relative link in the allreduce README points to the wrong location
because it does not go far enough up to the project root. Update the markdown
link under the allreduce example so it uses the correct number of
parent-directory hops from the README location, ensuring it resolves to the
tests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce target instead of
an examples/tests path.
In `@tests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/README.md`:
- Line 89: The link in the allreduce README is relative to the wrong depth and
resolves to a missing target. Update the markdown reference in the README so it
climbs back to the repository root before pointing to the minimal allreduce
example, and verify the corrected path from this README location still reaches
examples/workers/l3/allreduce/.
In `@tests/st/a2a3/tensormap_and_ringbuffer/collectives/reduce_scatter/README.md`:
- Around line 12-19: The README description for reduce_scatter does not match
the helper’s integer-based contract and input sizing, so update the
“Input/Output” and “Golden Check” text to use the same `nranks * COUNT_PER_RANK`
per-rank input size and the exact integer formula used by the shared helper.
Refer to the reduce_scatter example text in the README and align the closed-form
result wording with the implementation so it no longer implies a conflicting
chunk size or arithmetic model.
🪄 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
Run ID: 34964106-4b1a-42ae-9d23-694839b2fa61
📒 Files selected for processing (56)
docs/comm-domain.mdexamples/workers/l3/README.mdexamples/workers/l3/all_to_all_distributed/README.mdexamples/workers/l3/all_to_all_distributed/main.pyexamples/workers/l3/all_to_all_distributed/test_all_to_all.pyexamples/workers/l3/allgather_distributed/README.mdexamples/workers/l3/allgather_distributed/test_allgather.pyexamples/workers/l3/allreduce/README.mdexamples/workers/l3/allreduce/main.pyexamples/workers/l3/allreduce_distributed/README.mdexamples/workers/l3/allreduce_distributed/main.pyexamples/workers/l3/allreduce_distributed/test_allreduce.pyexamples/workers/l3/allreduce_ring_distributed/main.pyexamples/workers/l3/broadcast_distributed/README.mdexamples/workers/l3/broadcast_distributed/main.pyexamples/workers/l3/broadcast_distributed/test_broadcast.pyexamples/workers/l3/reduce_scatter_distributed/README.mdexamples/workers/l3/reduce_scatter_distributed/__init__.pyexamples/workers/l3/reduce_scatter_distributed/main.pyexamples/workers/l3/reduce_scatter_distributed/test_reduce_scatter.pysimpler_setup/kernel_compiler.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/__init__.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/_helpers.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/all_to_all/README.mdtests/st/a2a3/tensormap_and_ringbuffer/collectives/all_to_all/__init__.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/all_to_all/kernels/aiv/all_to_all_kernel.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/all_to_all/kernels/orchestration/all_to_all_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/all_to_all/test_all_to_all.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/allgather/README.mdtests/st/a2a3/tensormap_and_ringbuffer/collectives/allgather/__init__.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/allgather/kernels/aiv/allgather_kernel.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allgather/kernels/orchestration/allgather_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allgather/test_allgather.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/README.mdtests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/__init__.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/kernels/aiv/allreduce_bidirectional_ring_kernel.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/kernels/aiv/allreduce_ibing_kernel.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/kernels/aiv/allreduce_onephase_kernel.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/kernels/aiv/allreduce_ring_kernel.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/kernels/aiv/allreduce_twophase_kernel.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/kernels/orchestration/allreduce_bidirectional_ring_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/kernels/orchestration/allreduce_ibing_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/kernels/orchestration/allreduce_onephase_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/kernels/orchestration/allreduce_ring_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/kernels/orchestration/allreduce_twophase_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/test_allreduce.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/broadcast/README.mdtests/st/a2a3/tensormap_and_ringbuffer/collectives/broadcast/__init__.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/broadcast/kernels/aiv/broadcast_kernel.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/broadcast/kernels/orchestration/broadcast_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/broadcast/test_broadcast.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/reduce_scatter/README.mdtests/st/a2a3/tensormap_and_ringbuffer/collectives/reduce_scatter/__init__.pytests/st/a2a3/tensormap_and_ringbuffer/collectives/reduce_scatter/kernels/aiv/reduce_scatter_kernel.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/reduce_scatter/kernels/orchestration/reduce_scatter_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/collectives/reduce_scatter/test_reduce_scatter.py
💤 Files with no reviewable changes (20)
- examples/workers/l3/all_to_all_distributed/README.md
- examples/workers/l3/allgather_distributed/test_allgather.py
- examples/workers/l3/broadcast_distributed/README.md
- examples/workers/l3/reduce_scatter_distributed/init.py
- examples/workers/l3/allreduce_distributed/test_allreduce.py
- tests/st/a2a3/tensormap_and_ringbuffer/collectives/all_to_all/init.py
- examples/workers/l3/allreduce_distributed/README.md
- examples/workers/l3/reduce_scatter_distributed/README.md
- examples/workers/l3/all_to_all_distributed/test_all_to_all.py
- examples/workers/l3/allreduce_ring_distributed/main.py
- tests/st/a2a3/tensormap_and_ringbuffer/collectives/allgather/init.py
- examples/workers/l3/broadcast_distributed/test_broadcast.py
- tests/st/a2a3/tensormap_and_ringbuffer/collectives/init.py
- examples/workers/l3/allreduce_distributed/main.py
- tests/st/a2a3/tensormap_and_ringbuffer/collectives/allreduce/init.py
- examples/workers/l3/reduce_scatter_distributed/test_reduce_scatter.py
- examples/workers/l3/all_to_all_distributed/main.py
- examples/workers/l3/broadcast_distributed/main.py
- examples/workers/l3/reduce_scatter_distributed/main.py
- examples/workers/l3/allgather_distributed/README.md
d4fbf83 to
c7e80c1
Compare
|
Two items from review — both small but worth resolving before merge. 1. Orphaned dead doc:
|
|
Move the L3 collective algorithm corpus (allreduce with 5 modes, allgather, reduce_scatter, broadcast, all_to_all) from examples/workers/l3/ into tests/st/a2a3/tensormap_and_ringbuffer/collectives/ as @scene_test(level=3) classes. One class per mode×rank-count combination (19 test classes total). Shared comm-window helper (_helpers.py) provides DRY domain-allocation and orch-function building for all five ops. Kernel sources preserved via git mv. Add src/common/ to KernelCompiler.get_orchestration_include_dirs() so collective kernels can find platform_comm/comm_context.h. Delete old example dirs; update examples/workers/l3/README.md and docs/comm-domain.md. Add minimal onephase allreduce demo at examples/workers/l3/allreduce/ as the 'how to do a collective' feature. Algorithm documentation preserved in per-op READMEs under the new collectives/ directory (algorithm descriptions, phase diagrams, golden check formulas, test class tables). All 19 tests pass in pypto3-hw-native-sys:sim Docker (P2: 9/9, P4+ibing: 10/10). pre-commit clean (11/11 hooks pass).
…a2a3-only note - Remove orphaned examples/workers/l3/allreduce_distributed/README.md (directory was deleted; doc was unreachable and stale) - Make examples/workers/l3/allreduce/ self-contained: copy onephase kernel + orch locally instead of reaching into tests/ - Reframe examples/workers/l3/allreduce/README.md as an L3 Worker interface-usage demo showing the full register -> init -> allocate_domain -> submit_next_level -> run flow - Add tests/st/.../collectives/README.md documenting a2a3-only scope for collective scene tests Reviewed-by: Chao Wao
c7e80c1 to
2c01e88
Compare
|
@ChaoWao thanks for the thorough review — all four items addressed on 1. Orphaned 2. Silent a5 / a5sim test-coverage regression 3. 4. README reframed around the Also fixed the PR body test-class count (18, not 19). All 18 scene tests pass in sim Docker, pre-commit is clean (11/11). |
…rage
The collective scene tests are level=3 distributed worker tests, not the
level=2 single-chip cases that populate tests/st/{arch}/{runtime}/. Relocate
them out of the a2a3 arch bucket into a dedicated worker bucket that mirrors
their examples/workers/l3 origin and carries multi-platform CASES directly.
- git mv tests/st/a2a3/tensormap_and_ringbuffer/collectives ->
tests/st/worker/collectives (relative _helpers imports unchanged; no
__init__.py on worker/, matching the arch dirs)
- Restore the a5 coverage the prior relocation dropped, matching the pre-move
mainline declarations: 2-rank allreduce cases run on a5 onboard + a5sim,
4-rank and the other collectives (allgather, all_to_all, reduce_scatter,
broadcast) run on a5sim (a5 onboard CI exposes only 2 NPUs)
- Rewrite the collectives README note that claimed a2a3-only, and update the
path references in docs/comm-domain.md and examples/workers/l3
5a9fe46 to
d582711
Compare
…+ single-round-trip handshake (hw-native-sys#1310) Rebasing the polling-scheduler branch onto upstream/main pulled in three runtime changes the branch predates. Two needed no action — hw-native-sys#1308 (MIX per-core placement) patches classify_mix_cluster / prepare_block_for_dispatch, both removed by the polling redesign, and hw-native-sys#1294 only relocates tests. The other two require reconciliation, done here: - hw-native-sys#1268 renamed the profiling/DFX/timeout config macros to the SIMPLER_ prefix and removed the PTO2_ spellings. Update the three surviving references in redesigned code: PTO2_ORCH_PROFILING/PTO2_SCHED_PROFILING, PTO2_TENSORMAP_PROFILING, and PTO2_PROFILING -> SIMPLER_DFX. - hw-native-sys#1310 collapsed the AICore init handshake to a single round trip and dropped Handshake::aicore_regs_ready / aicpu_regs_ready. The AICore side (aicore_executor.cpp) and the Handshake struct are upstream-clean after the rebase, but the AICPU driver kept in the redesign (SchedulerContext::handshake_partition / emergency_shutdown, moved from scheduler_cold_path.cpp into scheduler_context.h) still ran the old three-phase protocol against the deleted fields. Port the single-round-trip transform: batch the Step 1 release barrier, sweep-poll aicore_done in Step 2 with platform_init_aicore_regs (DATA_MAIN_BASE=IDLE) doubling as the window-open acknowledgement, and drop the aicpu_regs_ready write from emergency shutdown. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Move the L3 collective algorithm corpus from
examples/workers/l3/intotests/st/a2a3/tensormap_and_ringbuffer/collectives/as@scene_test(level=3)classes._helpers.py): DRY comm-window domain-allocation and orch-function buildingexamples/workers/l3/allreduce/(onephase only) as "how to do a collective"src/common/toKernelCompiler.get_orchestration_include_dirs()forplatform_comm/comm_context.hTesting
pypto3-hw-native-sys:simDocker (P2 + P4 + ibing error)Files changed
49 files: +894 / −2032 (net −1138). Bulk of churn is kernel source renames + old example deletions. Net-new logic: ~280 lines in
_helpers.py+ ~200 lines across 5 test files.