Repository navigation
[BugFix][Dashboard] Render external Ascend optimization records - #358
Conversation
Connect hash-bound external records to native candidate views, protocol measurements, quality results, workflow events and source freshness. Distinguish empty campaigns and preserve unverified comparison boundaries. Agent-assisted-by: Codex (author) Signed-off-by: jiangkuaixue123 <jiangxiaozhou111@163.com>
|
Review of the full PR diff: base The overall design (hash-bound external records, verdicts shown but never fed into the generic gain ranking, explicit
Not approving yet; I'll re-review on the next push. |
|
@david6666666 麻烦帮忙 review 一下这个 PR,谢谢!已转为 Ready,主要修复 Ascend 外部优化记录无法进入原生 Dashboard 的问题,包括分档双轮指标、独立性能/质量判定、技术事件和采集状态。相关回归测试 91 项通过;全仓剩余 4 项失败已在未修改基线上复现,详情见 PR 描述。 |
Dong1017
left a comment
There was a problem hiding this comment.
Incremental review of the new mixed native/external dashboard path at this head. One additional, independently identified UI counting issue below; the previously raised localization, float/rounding, zero-metric, and CI questions remain in their existing discussion and are not repeated. Scope: static inspection of the changed dashboard template and its producer (dashboard_data.py); no tests, hardware runs, or experiment validation performed. This is a COMMENT review, not an acceptance or merge decision.
Translate integration messages independently, tolerate serialization roundoff, explain records without metrics and keep sample roles stable in charts. Cover empty snapshots and language switching with regression tests. Agent-assisted-by: Codex (author) Signed-off-by: jiangkuaixue123 <jiangxiaozhou111@163.com>
Agent-assisted-by: Codex (author) Signed-off-by: jiangkuaixue123 <jiangxiaozhou111@163.com>
|
Re-review of incremental changes: Thanks for the follow-up — most of my previous points look addressed on this head:
Still blocking for my approve (also called out by @Dong1017 on
Not approving yet; happy to re-review once that mixed-campaign counting path is fixed or explicitly specified with a test. |
Scope native failures and rollbacks to native runs and expose recorded external performance failures in a separate card. Cover mixed, native-only, external-only and empty campaign rendering. Agent-assisted-by: Codex (author) Signed-off-by: jiangkuaixue123 <jiangxiaozhou111@163.com>
|
@zengchuang-hw review |
amy-why-3459
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed base e843b9d31cb86e1fd12d00890c89798b0bedd1fe and head aeb2c7f79f30e336bd1c08ce00df5862d247f1e4 (incremental from my prior review at d2752da6f28147891136340a524f5cbdb546b9a2).
Verified the mixed-campaign fix in renderStats:
- native failures/rollbacks are scoped to rows without external metrics;
- mixed campaigns add a separate Recorded external performance failures card keyed off
performance_verdict; - native-only / external-only presentation is unchanged;
- zh strings for the new labels are present;
test_mixed_campaign_counts_native_and_external_failures_separatelyexercises the realrenderStatsbody across mixed / native-only / external-only / empty cases.
Ready CI on this head is green (run 42041734376), including full-check 3.10/3.11.
zengchuang-hw
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Full review of the eight-file e843b9d3..aeb2c7f7 change at head aeb2c7f79f30e336bd1c08ce00df5862d247f1e4, following the explicit review request. I checked Amy's initial review and her re-review at d2752da6, including the final d2752da6..aeb2c7f7 mixed-count fix. Assessed current target main 01c73521ced6ca760cdfa42c9b937fe985a20d64: clean effective merge tree 72c946a96963988c07facbca38c7c6d2ac0aa2df. Conclusion: no outstanding content blocker in the bounded #356 external-record/dashboard repair.
本轮已解决
- The mixed-campaign finding is fixed. Native failure/regression/rollback counts use native rows; a separate card counts external performance FAIL, including mixed campaigns and external records without measurements. The actual
renderStatsregression was independently exercised. - The earlier localization, floating-point midpoint tolerance, missing-metric visibility, role-specific chart colors and empty-campaign points are covered by the current code and regression tests.
- Reader validation binds candidate/source hashes and rejects conflicting native-run bindings or corrupt records. External performance/quality states remain separate, and external PASS does not enter generic verified-gain ranking. Ascend identity still requires the relevant recorded runtime fields.
批准前必做(Before I can approve)
None for this reviewed software change.
非阻塞问题或建议
No new suggestion. Collector/backend rollout and unrecorded historical narrative remain separate scope, as described in the PR.
Checks and limits
Independent exact-head review: 100 Dashboard tests passed, including Node-backed UI checks; changed-Python Ruff and whole-diff whitespace checks passed. A separate focused mixed-count run also passed.
Current main leaves the eight PR files and Dashboard tests unchanged. Its new trial qualification returns improvement_pct: null for unqualified comparisons; Dashboard already consumes that value rather than recomputing gain. Effective-merge verification added 20 passing tests / 1 deselected and a combined native-unqualified + external-FAIL case: no native gain or best result is invented, while external FAIL and the per-tier plot remain visible.
Current-head Ready CI and required policy passed. This review verifies software and test behavior; it does not certify caller-declared experiment validity, real Ascend execution or whole-Issue completion. I did not rerun the author's private deployment/browser or model campaign.
Direct delegation: the account holder zengchuang-hw authorized Codex in this chat to independently review other authors' InferRSI PRs and submit formal Approve after content blockers are cleared; the request is also recorded in the linked self-@ comment. Executor: Codex, with separate full-content and mixed-count cross-checks; acting GitHub account: zengchuang-hw, with repository write access or higher. This account/task did not author, materially contribute to or parent this delivery slice. Authorized scope/action: this PR's Dashboard diff, current-target integration and applicable verification, revision-bound APPROVE.
Governing Issue and delivery slice
Refs #356
Knowledge authorship declaration
Not applicable: no protected knowledge content is changed. Agent-assisted implementation is attributed in the signed-off commit.
Changed artifacts and evidence
Checks and dependencies
ruff check src tests toolspassed.git diff --checkpassed.Remaining work and Issue closure
Review follow-up (head d2752da)
Addresses the six review points:
1e-9/ absolute tolerance1e-12, documented in the contract. Tests accept binary/serialization roundoff and reject material differences; caller values remain unchanged.measurement(undefined)renders “Not recorded”.overview.comparison_state: gain eligibility, static chart, timeline and template. Charts/timeline only acceptcomparable; no separate CLI/report enum consumer was found. An empty-campaign snapshot test coversno_data, absent best gain/target gap, and both empty charts; the chart note now explicitly says no measurements recorded.The local loopback deployment has been restarted with the review fixes; API checks confirm existing records and the missing-metrics note remain available.
Mixed-campaign review follow-up (aeb2c7f)
Addresses Dong1017's inline feedback and amy-why-3459's re-review.
Mixed campaigns now display Native failed / regressed (native outcomes and rollbacks only) and a separate Recorded external performance failures card (external performance verdicts only). An external FAIL with an unverified outcome therefore remains visible without being promoted to native verified evidence. Native-only and external-only presentation remains unchanged; new labels include Chinese translations.
A Node regression executes the actual renderStats implementation with one native candidate plus an external FAIL, native failure / quality failure / regression / rollback variants, external PASS / INCONCLUSIVE / missing verdicts, and native-only / external-only / empty campaigns. All 100 dashboard tests and ruff checks pass. The local live dashboard serves the updated template. Latest-head CI and independent re-review remain pending; no reviewer thread is marked approved or resolved by the author.