Fix: remove redundant scene run configuration - #1915
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
💤 Files with no reviewable changes (10)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change makes AICPU thread configuration optional by default. Documentation and testing guidance now use architecture defaults. Examples and scene tests remove unnecessary thread and block-dimension overrides. Runtime helpers accept cases without a ChangesRuntime Configuration Defaults
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change removes redundant scene settings and keeps a specialized thread override; no actionable merge-blocking risk remains after normal checks, though the owner should confirm that the two-thread setting is required. Suggested labels: 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 |
8bc28c2 to
6f20709
Compare
|
整体方向认同： 建议本 PR 内处理1.
|
| 行 | 现文 | 应为 |
|---|---|---|
:55-56 |
"the common four-thread configuration uses 3 schedulers" | a5 默认五线程 / 4 调度器 |
:545-556 |
"With aicpu_thread_num=4…"，3 调度线程 × 12 AIC + 24 AIV（36÷3） |
4 调度线程 × 9 AIC + 18 AIV（36÷4） |
:909-913 |
RUNTIME_CONFIG = {"aicpu_thread_num": 4, "block_dim": 24} |
机制已废弃 + 线程数应为 5 + block_dim: 24 是 a2a3 的 PLATFORM_MAX_BLOCKDIM，a5 是 36 |
:545 尤其说明问题：cluster 数已经从 24 改成了 a5 的 36，唯独线程数没改 —— 和本 PR 修的那 6 个 a5 用例成因一致。
4. src/common/task_interface/call_config.h:59 —— 同一文件内两条注释互相矛盾
:22 * There is no block_dim knob: a run always takes the whole device.
:59 // ...distinct configuration tier from the top-level execution knobs (block_dim, aicpu_thread_num).
#1309 删字段时漏了 :59，删掉 block_dim, 即可。
5. tests/st/a5/tensormap_and_ringbuffer/dfx/chip_swimlane/test_chip_swimlane.py:71（可选）
注释写 "the low-thread cases exercise a shard layout the 4-thread default never reaches" —— 与 a2a3 同名文件逐字相同，在 a5 上默认是 5。该文件本 PR 未改动，顺带修与否都可以。
建议后续单独 PR
6. 给 CASES[*]["config"] 加未知键校验 —— 这是 block_dim 反复回潮的根因
配置分两层，只有下层有类型：CallConfig（C++ POD + nanobind）删得很干净，直接赋值会报错（上面第 1 条就是撞上它）；但上层 CASES[*]["config"] 是裸 dict，_build_config() 用一串 dict.get(k, default) 白名单拉取 —— 只问"有没有我认识的键"，从不问"有没有我不认识的键"，未知键连被看一眼的机会都没有。
后果：#1309 删字段并清理了 213 处，此后 18 天内被 8 个 PR 重新写回 8 次（#1464 就在删除当天，另有 #1473 / #1451 / #1540 / #1444 / #1678 / #1733 / #1819）。原因很朴素 —— 新增 scene test 的标准做法是复制现成文件，而没有任何东西会报错。本 PR 是第二次人肉清理。
而且 block_dim 还算最轻的（空转）。同一机制下：
ring_heap拼成ring_heep→ 静默取 0 → 落到PTO2_RING_*环境变量 / 编译期默认这条 fallback 链 → 测试照常通过，但跑的是完全不同的 ring 尺寸；device_count拼错 → 静默按 1 卡跑，双卡测试变单卡测试，依然绿。
这两类是"在错误配置下通过"，比死键危险得多。
设计上有几点已经查实，可以直接用：
config/runtime_env命名空间完全由框架拥有（全仓库没有测试从case["config"]读自定义键），可以严格校验。合法键集：aicpu_thread_num/runtime_env/device_count/num_sub_workers，runtime_env子键ring_task_window/ring_heap/ring_dep_pool。- case 顶层不能严格校验 ——
required_sched_phases是测试自有的合法扩展键（a5/.../test_chip_swimlane.py:116真实消费）。要覆盖顶层需要额外的EXTRA_CASE_KEYS之类声明机制。 - 拦截点建议放在
@scene_test装饰器（scene_test.py:1246，目前对CASES零校验）：import 期失败，一次覆盖 pytest 收集（_select_cases，:841）和 standalonerun_module（:1698）两条路径。 - 合法键集目前散在两个消费者（
_build_config():1386/:1392 与 session 分组 :1986/2097/2269/2273），从未写在一处 —— 校验器第一步就是把它收拢成单一事实源。
时机很好：全仓库 AST 扫描下来，config 层唯一的未知键就是 block_dim（8 处）。本 PR 清完后存量归零，校验器可以直接上，不会打出一堆历史失败。
7. 给保留下来的 aicpu_thread_num 覆盖补注释
本 PR 在 .claude/skills/testing/SKILL.md:185 新增了"必须覆盖时加就近注释说明依赖"的规则，但保留的绝大多数覆盖（dummy_task / predicated_dispatch / dep_gen_chain 等的 =2）都没有注释，新规则落地即处于自相矛盾状态。scope_stats 和 chip_swimlane 是有注释的正面例子。至少在 PR 描述里记为后续项。
6f20709 to
d48811d
Compare
|
@ChaoZheng109 感谢详细核对，1–7 项都已在本 PR 一并处理：
另外同步修正了 testing 文档/skill 中的合法键说明，并完成 A2/A3、A5 全量 sim sweep 与 pre-commit。 |
d48811d to
9ace656
Compare
- Drop default aicpu_thread_num and obsolete top-level block_dim from ordinary HBG/TMR scene cases, examples, and the prewarm benchmark
- Reject unknown CASES config and runtime_env keys when scene-test classes load
- Keep topology-specific thread counts and document what each override covers
- Use case.get("config", {}) where whitebox helpers assumed config existed
- Update scene-test guidance and A5 scheduler topology documentation
2 participants
Summary
aicpu_thread_numand obsolete top-levelblock_dimfrom ordinary HBG/TMR scene cases, examples, and the prewarm benchmarkCASES[*]["config"]and nestedruntime_envkeys when scene-test classes are importedCASES[*]["config"]always existed after config cleanupTest plan
5 passed)worker.run()withoutAttributeError1681 passed, 6 skipped; two GCC setup tests are incompatible with the locally installed/usr/bin/g++-15, and one worker child-reaping case passed on isolated rerunnpu-smiis unavailable on this machine