Refactor: remove the user-facing block_dim knob - #1309
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
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:
📝 WalkthroughWalkthroughThis change updates Changesblock_dim configuration reset
Estimated code review effort: 1 (Trivial) | ~5 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.
🧹 Nitpick comments (1)
tests/st/a2a3/tensormap_and_ringbuffer/spmd_basic/test_spmd_basic.py (1)
74-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCase1 and Case2_AutoBlockDim are now functionally equivalent.
Case1 explicitly sets
block_dim: 0and Case2_AutoBlockDim omitsblock_dim(defaulting to 0 via_build_config). Both resolve to the sameCallConfig, making the two cases redundant in terms of runtime configuration. If the intent is to test both the explicit-0 and omitted paths as distinct regression cases, consider adding a comment to Case1 clarifying that distinction. Otherwise, Case2_AutoBlockDim could be removed.🤖 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/st/a2a3/tensormap_and_ringbuffer/spmd_basic/test_spmd_basic.py` around lines 74 - 86, Case1 and Case2_AutoBlockDim are redundant because both end up using the same CallConfig through block_dim=0. Update the table entry in test_spmd_basic to either remove Case2_AutoBlockDim or add a clear comment near Case1 and Case2 explaining that one exercises the explicit block_dim=0 path and the other exercises the omitted-field path via _build_config, so the distinction is intentional.
🤖 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.
Nitpick comments:
In `@tests/st/a2a3/tensormap_and_ringbuffer/spmd_basic/test_spmd_basic.py`:
- Around line 74-86: Case1 and Case2_AutoBlockDim are redundant because both end
up using the same CallConfig through block_dim=0. Update the table entry in
test_spmd_basic to either remove Case2_AutoBlockDim or add a clear comment near
Case1 and Case2 explaining that one exercises the explicit block_dim=0 path and
the other exercises the omitted-field path via _build_config, so the distinction
is intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a577d69-2578-4554-831e-3134c73b3635
📒 Files selected for processing (26)
examples/a2a3/tensormap_and_ringbuffer/benchmark_bgemm/test_benchmark_bgemm.pyexamples/a2a3/tensormap_and_ringbuffer/paged_attention/test_paged_attention.pyexamples/a2a3/tensormap_and_ringbuffer/paged_attention_manual_scope/test_paged_attention.pyexamples/a2a3/tensormap_and_ringbuffer/paged_attention_ringbuffer/test_paged_attention_ringbuffer.pyexamples/a2a3/tensormap_and_ringbuffer/paged_attention_unroll_manual_scope/test_paged_attention_unroll.pyexamples/a5/tensormap_and_ringbuffer/paged_attention_unroll_manual_scope/test_paged_attention_unroll.pytests/st/a2a3/host_build_graph/bgemm/test_bgemm.pytests/st/a2a3/host_build_graph/paged_attention/test_paged_attention.pytests/st/a2a3/tensormap_and_ringbuffer/alternating_matmul_add/test_alternating_matmul_add.pytests/st/a2a3/tensormap_and_ringbuffer/batch_paged_attention/test_batch_paged_attention.pytests/st/a2a3/tensormap_and_ringbuffer/fanin_lookup_perf/test_fanin_lookup_perf.pytests/st/a2a3/tensormap_and_ringbuffer/multi_round_paged_attention/test_multi_round_paged_attention.pytests/st/a2a3/tensormap_and_ringbuffer/paged_attention_unroll/test_paged_attention_unroll.pytests/st/a2a3/tensormap_and_ringbuffer/paged_attention_unroll_4dims/test_paged_attention_unroll_4dims.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_basic/test_spmd_basic.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_batch_dispatch_oob/test_spmd_batch_dispatch_oob.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_multiblock_aiv/test_spmd_multiblock_aiv.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_multiblock_mix/test_spmd_multiblock_mix.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_paged_attention/test_spmd_paged_attention.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_paged_attention_highperf/test_spmd_paged_attention_highperf.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_starvation/test_spmd_starvation.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start/test_spmd_sync_start.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_aiv/test_spmd_sync_start_aiv.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_edge/test_spmd_sync_start_edge.pytests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_stress/test_spmd_sync_start_stress.pytests/st/a5/tensormap_and_ringbuffer/paged_attention_unroll/test_paged_attention_unroll.py
ab27c98 to
ef79342
Compare
ef79342 to
af8ab64
Compare
af8ab64 to
68af59e
Compare
|
Heads-up: #1458 landed and it invalidates this PR's core premise on sim. This PR's rationale is that
That is no longer true. #1458 added So on sim this stops being a no-op cleanup, and at least two cases break
The fix is the rework this PR's follow-up was always going to need: SPMD Until then, please don't merge this as-is on top of current main — the two |
ff015ba to
775d280
Compare
|
Heads-up: I've force-pushed onto this branch, replacing What changed relative to your version Two things, one additive and one a design swap. Additive: the PR now also removes the Design swap:
With the device reporting its own width there is nothing to keep in sync and no State: in progress — 4 of 6 sync_start cases converted ( |
8514da0 to
8a54fed
Compare
573c75b to
6ebdec1
Compare
block_dim let a caller pin how many AICore blocks a task occupied, but the value is a property of the device, not of the task: every in-repo caller either left it at the "auto" sentinel or pinned the width of the one device it was written for. Pinning it below the device width simply wasted cores, and pinning it above was rejected at run time, so the knob only ever expressed what the runtime already knows. - Drop CallConfig::block_dim and its plumbing through the task interface, the packed mailbox wire layout, the remote-L3 protocol, and both platform device runners; the runner now always resolves the block count from the stream's capacity. - Strip the pinned "block_dim" entries from the scene tests and examples that carried them, and update the fixtures that constructed a CallConfig with one. The wire contract was asserted in three places — the header's static_assert, a C++ unit test that restated the same sizeof expression, and a remote-wire round-trip that set the field — and all three move with the layout. - The two spmd_sync_start_mix_spill docstrings stop quoting core counts. They named a5's 72 AIV / 24 clusters and a2a3's 48 / 24, which were already inconsistent with the block_dim those cases pinned and are meaningless once the width is the device's own. (The a5 one also carried a paragraph twice.) - available_aicore_counts loses its Pinned case: it fixed block_dim so the host held an expectation independent of the reported count, which was the only way to catch an under-report. Nothing replaces it, and the docstrings say so rather than implying coverage that is gone. - Sweep the docs and skills that documented the knob. Co-authored-by: majin0824 <majin15@huawei.com>
…tree An editable install pins the compiled extension at install time (`editable.rebuild = false`) while `python/simpler/*.py` is read live. Switching branches or rebasing therefore moves the Python out from under a fixed binary, nothing rebuilds, and a changed struct layout makes attributes read as 0 with no error anywhere. That is not hypothetical. A worktree installed before hw-native-sys#1309 (which dropped `block_dim` from `CallConfig`) and then branched onto a main containing it read `aicpu_thread_num` as 0, and the whole onboard L2 suite failed with `launch_aicpu_num (0) must be in range [1, 4]` — a plausible-looking runtime rejection that reads as a product bug. It cost a full bisect, and an A/B against main "reproduced" it because both arms shared the same stale binary. The same skew hit again a few hours later as `AttributeError: run_stream_set_create_count` after a rebase across hw-native-sys#1464. `_task_interface` now records the commit it was built from, and `simpler.task_interface` compares it against the working tree at import, raising with the one command that fixes it. Loud beats silent here: the alternative is not an error, it is wrong values. A warning would also have been the wrong choice — pytest relegates import-time warnings to its end-of-run summary, and in the failure above the same warning would have appeared in *both* arms of the A/B and been dismissed as noise. A *missing* stamp raises too, rather than being treated as "cannot tell". The attribute is absent only on an extension compiled before it existed, which in a checkout new enough to run the check is by definition a different revision — and is the state of every already-installed worktree the day this lands. Keyed on git HEAD, matching `RuntimeBuilder._build_cache_stamp` — mtimes do not survive a branch switch, which is why that stamp is git-based too. Deliberately the whole HEAD rather than a subset: excluding paths means maintaining a second "what cannot affect the build" list beside the one CI already keeps, and the guard exists precisely so correctness does not rest on such judgments. It costs little — 120 of the last 200 commits touch the ABI surface anyway, so most of the reinstalls it forces were owed regardless and merely became visible; the rest cost one ~20 s reinstall. Inert outside a source tree: a wheel has no `.git` to compare against, and a build made without git carries an empty stamp. The rebuild table gained the trigger it was missing and no longer contradicts itself. It was framed as "what did you change", so it had no row for the case where you changed nothing and the tree moved — the one that bites, and the one where verifying that `import simpler` resolves into your worktree gives false confidence. Its "no rebuild needed" rows now say what is actually true: nothing to recompile, but the guard keys on HEAD, so a commit still needs a reinstall before the next import. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tree An editable install pins the compiled extension at install time (`editable.rebuild = false`) while `python/simpler/*.py` is read live. Switching branches or rebasing therefore moves the Python out from under a fixed binary, nothing rebuilds, and a changed struct layout makes attributes read as 0 with no error anywhere. That is not hypothetical. A worktree installed before hw-native-sys#1309 (which dropped `block_dim` from `CallConfig`) and then branched onto a main containing it read `aicpu_thread_num` as 0, and the whole onboard L2 suite failed with `launch_aicpu_num (0) must be in range [1, 4]` — a plausible-looking runtime rejection that reads as a product bug. It cost a full bisect, and an A/B against main "reproduced" it because both arms shared the same stale binary. The same skew hit again a few hours later as `AttributeError: run_stream_set_create_count` after a rebase across hw-native-sys#1464. `_task_interface` now records the commit it was built from, and `simpler.task_interface` compares it against the working tree at import, raising with the one command that fixes it. Loud beats silent here: the alternative is not an error, it is wrong values. A warning would also have been the wrong choice — pytest relegates import-time warnings to its end-of-run summary, and in the failure above the same warning would have appeared in *both* arms of the A/B and been dismissed as noise. A *missing* stamp raises too, rather than being treated as "cannot tell". The attribute is absent only on an extension compiled before it existed, which in a checkout new enough to run the check is by definition a different revision — and is the state of every already-installed worktree the day this lands. Keyed on git HEAD, matching `RuntimeBuilder._build_cache_stamp` — mtimes do not survive a branch switch, which is why that stamp is git-based too. Deliberately the whole HEAD rather than a subset: excluding paths means maintaining a second "what cannot affect the build" list beside the one CI already keeps, and the guard exists precisely so correctness does not rest on such judgments. It costs little — 120 of the last 200 commits touch the ABI surface anyway, so most of the reinstalls it forces were owed regardless and merely became visible; the rest cost one ~20 s reinstall. Inert outside a source tree: a wheel has no `.git` to compare against, and a build made without git carries an empty stamp. The rebuild table gained the trigger it was missing and no longer contradicts itself. It was framed as "what did you change", so it had no row for the case where you changed nothing and the tree moved — the one that bites, and the one where verifying that `import simpler` resolves into your worktree gives false confidence. Its "no rebuild needed" rows now say what is actually true: nothing to recompile, but the guard keys on HEAD, so a commit still needs a reinstall before the next import. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tree (#1523) An editable install pins the compiled extension at install time (`editable.rebuild = false`) while `python/simpler/*.py` is read live. Switching branches or rebasing therefore moves the Python out from under a fixed binary, nothing rebuilds, and a changed struct layout makes attributes read as 0 with no error anywhere. That is not hypothetical. A worktree installed before #1309 (which dropped `block_dim` from `CallConfig`) and then branched onto a main containing it read `aicpu_thread_num` as 0, and the whole onboard L2 suite failed with `launch_aicpu_num (0) must be in range [1, 4]` — a plausible-looking runtime rejection that reads as a product bug. It cost a full bisect, and an A/B against main "reproduced" it because both arms shared the same stale binary. The same skew hit again a few hours later as `AttributeError: run_stream_set_create_count` after a rebase across #1464. `_task_interface` now records the commit it was built from, and `simpler.task_interface` compares it against the working tree at import, raising with the one command that fixes it. Loud beats silent here: the alternative is not an error, it is wrong values. A warning would also have been the wrong choice — pytest relegates import-time warnings to its end-of-run summary, and in the failure above the same warning would have appeared in *both* arms of the A/B and been dismissed as noise. A *missing* stamp raises too, rather than being treated as "cannot tell". The attribute is absent only on an extension compiled before it existed, which in a checkout new enough to run the check is by definition a different revision — and is the state of every already-installed worktree the day this lands. Keyed on git HEAD, matching `RuntimeBuilder._build_cache_stamp` — mtimes do not survive a branch switch, which is why that stamp is git-based too. Deliberately the whole HEAD rather than a subset: excluding paths means maintaining a second "what cannot affect the build" list beside the one CI already keeps, and the guard exists precisely so correctness does not rest on such judgments. It costs little — 120 of the last 200 commits touch the ABI surface anyway, so most of the reinstalls it forces were owed regardless and merely became visible; the rest cost one ~20 s reinstall. Inert outside a source tree: a wheel has no `.git` to compare against, and a build made without git carries an empty stamp. The rebuild table gained the trigger it was missing and no longer contradicts itself. It was framed as "what did you change", so it had no row for the case where you changed nothing and the tree moved — the one that bites, and the one where verifying that `import simpler` resolves into your worktree gives false confidence. Its "no rebuild needed" rows now say what is actually true: nothing to recompile, but the guard keys on HEAD, so a commit still needs a reinstall before the next import.
What
A run now always takes the whole device.
block_dimwas a per-call knob thatevery scene test pinned to the platform maximum anyway, and it forced host
orchestration to guess a width before the device had reported one.
The knob, end to end
CallConfigloses the field. The packed wire layout is one int32 shorter,so the remote-L3
PROTOCOL_VERSIONgoes 1 → 2 in both codecs and the forkmailbox format string drops an
i. The compile-time layout guard incall_config.his updated with it — it is what caught the drift.DeviceRunnerresolves the width unconditionally: every cluster theAICore stream reports onboard,
SIM_AUTO_BLOCKDIMon sim.scene_test's config plumbing, and thel0_swimlanereplay lose their
block_dimpaths.Two latent problems surfaced while doing it and are fixed here:
remote_l3_session.pyhardcodedprotocol_version=1in the HELLO payloadinstead of referencing the constant — a version bump would have silently
missed it.
spmd_basic'sCase2_AutoBlockDimbecame an exact duplicate ofCase1oncethe knob was gone, so it is dropped.
Scene tests
178 pinned values across 75 files removed. Kernel-side
params["block_dim"]— atiling argument, not a runtime knob — is untouched.
l0_swimlanecan no longer infer an SPMD replay width from the test file, sincea cohort now sizes itself on device:
--spmd-block-numis required to replayone, and the help/doc say so.
Coverage note
Sim resolves to
SIM_AUTO_BLOCKDIM(8) and onboard to the device's real width(24 on a2a3), so sim no longer exercises wide-device behaviour. That is a
deliberate trade — sim runs one OS thread per AICore, and 24-36 clusters is
72-108 threads per case — but it is why #1477's bug was invisible until an
onboard run. Wide-device coverage rests on the onboard job.
Verification
Measured on top of #1478 (#1477 is now in main):
pytest examples tests/st --platform a2a3(onboard)pytest examples tests/st --platform a2a3simpytest examples tests/st --platform a5simpytest tests/ut -m "not requires_hardware"The four cases that keep
aicpu_thread_num: 2(dummy_task,predicated_dispatchon both runtimes,dep_gen_chain) pass at the full24-cluster width with no thread-count workaround, which is the check that #1477
actually fixed the cause rather than moving it.
History
This PR started as "zero out the hardcoded
block_dim". That premise stoppedholding when #1458 landed
SIM_AUTO_BLOCKDIM = 8: on sim the auto sentinel nolonger resolves to
PLATFORM_MAX_BLOCKDIM, so zeroing the pins is a behaviourchange rather than a no-op. Removing the knob outright makes "a run takes the
whole device" the only shape there is. @doraemonmj's cohort-width analysis lives
on in #1478.
Cost: the under-count check from #1472
#1472 added a
Pinnedcase to the threeavailable_aicore_countstests thatfixed
block_dimso the host held an expectation independent of what the devicereported — the only thing able to catch an under-reported count. That handle
is exactly the knob this PR removes, so the case goes with it.
What survives: the count is range-checked,
aiv == 2 * clustersis checked, andthe cohort is actually spent, so an over-reported count still trips the
require_sync_startdeadlock guard on device. What is lost: an under-reportedcount is now self-consistent — fewer blocks launch, fewer slots are expected,
the tail is zero on both sides. The test docstrings say so rather than implying
coverage that is not there.
Restoring it needs a host-side handle on the device width that is not a second
source of truth for a device property. There is not one today.