Add: restore marked args for L0 swimlane replay - #1567
Conversation
📝 WalkthroughWalkthroughAdds selective level-3 tensor payload capture and ChangesSelective dump and replay
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
simpler_setup/scene_test.py (1)
1703-1715: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--rounds > 1disable leaks stale payload-filter CLI args into dispatched children, crashing them.This block clears
args.dump_argsandargs.dump_args_payload_filterwhenrounds > 1, but notargs.dump_args_payload_func_id/args.dump_args_payload_slot. Those two raw attributes are what_dispatch_test_phases_standalone'scommonlist construction (Lines 1899-1904) actually forwards to spawned child subprocesses — independently ofargs.dump_args_payload_filter. Sinceif args.dump_args:is now false,--dump-argsis omitted fromcommon, but--dump-args-payload-func-id/--dump-args-payload-slotare still appended (their own guards only check the raw attrs). Each child re-parses argv and calls_resolve_dump_args_payload_filter(0, <leaked func-id>, <leaked slots>), which raisesValueError("... requires --dump-args 3")→parser.error()→SystemExit(2), failing every L2 fan-out job instead of silently disabling dump-args as intended (and as already happens correctly on the pytest side, and on the single-group inline-execution path which reuses the already-clearedargs.dump_args_payload_filter).🐛 Proposed fix
if args.rounds > 1 and args.dump_args: logger.warning("Dump args disabled: --rounds > 1") args.dump_args = 0 args.dump_args_payload_filter = None + args.dump_args_payload_func_id = None + args.dump_args_payload_slot = []🤖 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 `@simpler_setup/scene_test.py` around lines 1703 - 1715, When disabling dump-args for args.rounds > 1 in the argument-normalization block, also clear the raw args.dump_args_payload_func_id and args.dump_args_payload_slot attributes used by _dispatch_test_phases_standalone when building its common child-argument list. Keep the existing dump_args_payload_filter reset so spawned children receive no stale payload-filter options and dump-args remains fully disabled.
🤖 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/dfx/l0-swimlane-profiling.md`:
- Around line 683-685: Update the replay-data statement in the profiling
documentation to say that only unselected tensor payloads are zero-filled;
preserve the clarification that scalar values and tensor descriptors retain
dump-derived values.
- Around line 71-72: Update the tensor restoration scope statement in the
profiling documentation to include both a2a3 and a2a3sim, while preserving the
existing availability of a5 metadata-only and zero-filled replays.
In `@src/a5/platform/sim/host/device_runner.cpp`:
- Around line 694-697: Update the A5 initialization path in device_runner around
dump_collector_.initialize to prevent selective payload capture: pass the
disabled payload-filter value instead of dump_args_payload_filter_, or reject
enabled filters with a clear error before initialization. Preserve the existing
collector setup for all other dump options.
---
Outside diff comments:
In `@simpler_setup/scene_test.py`:
- Around line 1703-1715: When disabling dump-args for args.rounds > 1 in the
argument-normalization block, also clear the raw args.dump_args_payload_func_id
and args.dump_args_payload_slot attributes used by
_dispatch_test_phases_standalone when building its common child-argument list.
Keep the existing dump_args_payload_filter reset so spawned children receive no
stale payload-filter options and dump-args remains fully disabled.
🪄 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: d4123f79-b2b6-4407-8c63-b7d6ec6bba38
📒 Files selected for processing (30)
.claude/skills/l0-swimlane/SKILL.mdconftest.pydocs/dfx/args-dump.mddocs/dfx/l0-swimlane-profiling.mdpython/bindings/task_interface.cpppython/simpler/worker.pysimpler_setup/scene_test.pysimpler_setup/tools/dump_viewer.pysimpler_setup/tools/l0_swimlane.pysrc/a2a3/platform/onboard/host/device_runner.cppsrc/a2a3/platform/sim/host/device_runner.cppsrc/a5/platform/onboard/host/device_runner.cppsrc/a5/platform/sim/host/device_runner.cppsrc/common/platform/include/common/args_dump.hsrc/common/platform/include/host/args_dump_collector.hsrc/common/platform/onboard/host/device_runner_base.cppsrc/common/platform/onboard/host/device_runner_base.hsrc/common/platform/shared/aicpu/args_dump_aicpu.cppsrc/common/platform/shared/host/args_dump_collector.cppsrc/common/platform/sim/host/device_runner_base.cppsrc/common/platform/sim/host/device_runner_base.hsrc/common/task_interface/args_dump_config.hsrc/common/task_interface/call_config.htests/st/a2a3/tensormap_and_ringbuffer/dfx/args_dump/test_args_dump.pytests/ut/cpp/a2a3/test_args_dump.cpptests/ut/cpp/common/test_args_dump_collector.cpptests/ut/cpp/types/test_call_config.cpptests/ut/py/test_chip_worker.pytests/ut/py/test_l0_swimlane.pytests/ut/py/test_scene_test_dump_args.py
3b07894 to
a029fd4
Compare
f33aba3 to
8a3bdee
Compare
设计建议:L3 的选择性 payload 应复用 L1 通路,而非新起一条并行过滤器整体实现质量很高(wire 结构静态断言齐全、双层校验、测试覆盖到位),但对 L3 的改造方向建议调整一下:这条"按 task/arg 槽位选 payload"的能力,L1(PARTIAL / 重复点
唯一的真实分歧点,以及它的解法复用 L1 的顾虑是:L1 的掩码门控的是"整条记录要不要产生"(元数据+payload 一起丢),直接套到 L3 会让 L3 的 JSON 不再全量。 解法很简单——设备侧
bool copy_payload_l3 = false;
if (g_dump_args_level == FULL_JSON_ONLY) {
ArgsDumpArgMask mask, flags;
get_dump_args_task_masks(info.task_id, &mask, &flags); // L1 已有的 per-task 查表
copy_payload_l3 = has_dump_arg_flag(mask, info.arg_index); // L1 已有的纯位测试(无模式短路)
}
if (is_scalar || (g_dump_args_level == FULL_JSON_ONLY && !copy_payload_l3)) {
copy_bytes = 0;
}
于是整个设计塌缩成
这样 tensor 掩码通路从头到尾就是 L1 的,L3 与 L1 的差别被压到"掩码管 record 还是只管 bin"这一个 level 分支上,语义清楚、surface 最小。 其余部分( |
补充:复用 L1 方案的覆盖范围复用 L1 后,L3 选择性 payload 的覆盖面直接继承 L1 的注入点现状。确认 scope 如下:
改动量:
a5 hbg 先不支持是明确的 scope 边界(要覆盖需先补齐 a5 hbg 的 orchestrator dump 通路,超出本 PR);且 a5 payload 消费本就被 #1560 标为不受支持,这个边界是自洽的。请在 PR 描述里显式写明,避免被当成遗漏。 |
indigo1973
changed the title
4ff360d to
f82e04a
Compare
indigo1973
changed the title
|
现在，匹配后通过现有的 set_dump_args_task_mask(task_id, mask, 0) 写入 L1 已有的 per-task mask table。L3的 dump_arg_record() 消费侧也改为使用 get_dump_args_task_masks() 和 has_dump_arg_flag()，mask 只控制 args.bin payload 是否复制，args_dump.json 的记录仍然完整输出。原有 L1 PARTIAL 模式的 record 过滤语义没有变化。原来的 per-record payload_filter_matches() 路径已经移除，因此不再存在第二套 payload 判定或 mask table。与建议字面上唯一的差异是没有完全删除 DumpArgsPayloadFilter。它现在只负责把 CLI 中尚未解析的“func-id 集合 + slot mask”从 host 传到 AICPU。因为此时还没有运行期 task_id，现有 L1 table 只能存放解析后的 task_id -> mask，无法直接携带这个 selector。selector 到达 orchestrator、解析成具体 task 后，后续数据通路完全复用 L1。 |
indigo1973
changed the title
Level 3 args dump keeps complete args_dump.json metadata for Core (L0) swimlane replay while reusing the existing Level 1 CoreTaskArgs::dump(...) mask to write only marked tensor payloads. This preserves Level 1 behavior and avoids a parallel task/slot selector path. - Add --restore-arg SLOT to initialize selected replay tensors from their captured before_dispatch bytes for structured inputs in hw-native-sys#1532. - Add --msprof-timeout MINUTES to bound op-simulator collection. - Keep args.bin optional when Level 3 has no marked tensor payload. - Reject a5 payload restoration while hw-native-sys#1560 remains open; completing that platform scope depends on fixing hw-native-sys#1560. - Cover payload capture, restoration, strided views, and error paths.
2 participants
Level 3 args dump keeps complete args_dump.json metadata for Core (L0)
swimlane replay while reusing the existing Level 1
CoreTaskArgs::dump(...) mask to write only marked tensor payloads. This
preserves Level 1 behavior and avoids a parallel task/slot selector path.
their captured before_dispatch bytes for structured inputs in #1532.
that platform scope depends on fixing #1560.