Fix: align HBG host and device swimlane timelines - #2244
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: Advanced 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: Advanced Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds persisted Host/device clock alignment for eligible single-file captures. It adds scheduler-stream containment, alignment metadata and fallback behavior, updated profiling documentation, diagnostic logging, and tests for legacy, HBG, device-only, and invalid-input cases. ChangesHost/device clock alignment
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Capture as Single-file capture
participant Converter as swimlane_converter
participant Logs as Host logs
participant Metadata as metadata.clock_alignment
Capture->>Converter: Prepare or reuse alignment
Converter->>Logs: Match Host records when no valid saved mapping exists
Logs-->>Converter: Host anchors and timing bounds
Converter->>Metadata: Persist validated mapping atomically
Converter-->>Capture: Decode with aligned or fallback timestamps
Merge Risk: ⚪ Minimal · up to The alignment, containment, fallback, and persistence paths have corresponding validation coverage, with no actionable merge-blocking issue identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 31.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 6 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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. A rabbit checks the clocks at dawn Comment |
d96a00e to
9f970f2
Compare
indigo1973
changed the title
zhusy54
left a comment
zhusy54
left a comment
There was a problem hiding this comment.
Review：必须修复项与测试精简
必须修复
-
Host logger 首次目录绑定与临时 capture 生命周期冲突
Worker._submit_locked会把本次output_prefix传给 Host logger，但 logger 采用第一个非空目录永久生效的策略；alignment exporter 又通过该目录路径重新打开host.<pid>.log。如果第一次 capture 目录被删除，第二次 capture 的重新绑定会被忽略，exporter 无法重新打开旧路径，成功的运行可能最终因 alignment export 失败返回PTO_RUNTIME_ERR_INTERNAL。另外，绑定发生在 callable/handle 验证前，一次失败 submit 也可能永久污染后续运行。建议把 persistent process log 放到独立于单次 capture 的稳定 process/session 目录，或实现安全的 sink rotation/rebind；同时避免失败 submit 永久改变 logger destination。
-
当前临时目录测试掩盖了上述问题
test_st_log_drain_survives_temporary_capture_directories使用drain_host_logfixture，而该 fixture 会预先把 logger 绑定到 pytest session 目录。因此测试中的_set_host_log_directory(capture)实际不会切换目录，只证明 fixture 的稳定目录可读，没有覆盖生产生命周期。请删除或重写该测试。最小有效回归应使用同一 Worker/ChipWorker：完成第一次 capture、删除第一次 capture 目录、执行第二次 capture，并验证第二次运行和 alignment export 均成功。
first_invocation=[1, 3]不触发不同分支，也可以删除。 -
TestConsecutiveClockAlignment的核心断言没有进入现有 CI该测试仅支持 A5、标记为 manual；per-PR A5 主 sweep 会排除 manual，onboard DFX action 又没有运行 A5 HBG chip-swimlane。daily 主 sweep没有传
--enable-chip-swimlane，因此测试会在level == 0时返回。如果保留该测试，请把它接入真正启用 HBG L3 swimlane 的 CI，并确保两个 invocation 都执行。
if len(invocations) == 2应改为强断言，避免只运行一个 case 时静默通过。若有更轻量且能真实覆盖目录删除的回归，可以用其替换当前 manual scene test，不需要保留两套测试。 -
directory 写回失败后，event 与输出 metadata 可能不一致
single-file 路径写回失败后会更新内存 raw metadata，directory 路径目前只打印 warning。如果输入已有旧 mapping，本次 event 使用新 placement，但输出
timeline_metadata.clock_alignment可能仍是旧 mapping；如果输入没有 mapping，输出 metadata 可能缺少本次实际使用的 mapping。这里重点是行为分叉，不是代码量。请保证内存中的 mapping 始终更新，源文件持久化仅为 best effort。测试只需一个最小回归：输入带旧 mapping、计算出新 mapping、模拟写回失败，断言 event 和输出 metadata 都使用新 mapping；无需扩展完整失败组合矩阵。
-
删除
scheduler_records.streams[].runtimedocument-level
metadata.runtime已足够，stream-levelruntime是重复数据。目前 producer、文档和 fixture 存在两种 stream shape。请删除 producer 中的该字段，同步测试和文档，并搜索 consumer 确认没有依赖。现有 producer/scene test 应显式断言runtime不在 stream 中，不需要新增独立测试文件。 -
恢复 TMR directory 覆盖，新增独立 HBG fixture
共享
_write_l3_rank从 TMR 改成 HBG 后，原有 multi-rank TMR directory merge 覆盖被整体替换。请恢复原 TMR fixture，并新增独立 HBG alignment fixture；保留一个 multi-rank TMR directory merge 用例即可，不需要扩展更多组合。
可精简的测试
-
_verify_runtime_clock_logs已检查 alignment log 数量、单一(pid, inv)和 required markers；TestConsecutiveClockAlignment后续重复检查了相同内容。建议前者只负责验证 converter 执行前 native artifact 已存在，后者只保留 PID/inv 与 placement 对应、anchor 和连续 invocation 等集成断言。 -
test_single_capture_persists_alignment...已精确断言 anchor 和转换后的 timestamp，dispatch_time_us > 1000以及重复的 bounds 包含关系断言信息增量较低，可删除；这项仅是精简建议，不作为合入阻塞条件。
本 review 不要求额外增加 A2/A3 hardware smoke、原任务失败时 flush 专项测试，或扩展更多 frequency/level/marker 组合。测试应聚焦于已确认的问题和本 PR 明确承诺的行为。
538af91 to
492ab96
Compare
Adapt Host Build Graph swimlane artifacts and conversion so Host records and Device records share a bounded timeline on A2/A3 and A5. - Export invocation-scoped host_clock_alignment.<pid>.log files beside eligible captures while keeping cumulative Host logs in a capture-independent process session spool that survives consecutive capture cleanup. - Derive and persist a fixed-size metadata.clock_alignment mapping from native timing markers, apply it to single-file and multi-rank conversion, and keep generated metadata consistent when source writeback fails. - Read the runtime from document metadata, support the structured HBG Scheduler records and TaskId layout, name AICore Scheduler lanes by worker_id, and omit redundant per-stream runtime fields. - Render TMR TaskIds with explicit ring labels, including ring zero, across swimlane, dependency, and critical-path views. - Cover runtime-produced alignment artifacts, consecutive captures, HBG mapping persistence, and the retained multi-rank TMR directory merge in CI and tests. Co-authored-by: majin0824 <majin15@huawei.com>
492ab96 to
5f9208d
Compare
2 participants
Adapt Host Build Graph swimlane artifacts and conversion so Host records and
Device records share a bounded timeline on A2/A3 and A5.
captures while keeping cumulative Host logs in a capture-independent process
session spool that survives consecutive capture cleanup.
timing markers, apply it to single-file and multi-rank conversion, and keep
generated metadata consistent when source writeback fails.
records and TaskId layout, name AICore Scheduler lanes by worker_id, and omit
redundant per-stream runtime fields.
swimlane, dependency, and critical-path views.
persistence, and the retained multi-rank TMR directory merge in CI and tests.
Co-authored-by: majin0824 majin15@huawei.com