Skip to content

[BugFix][Dashboard] Render external Ascend optimization records - #358

Merged
jiangkuaixue123 merged 4 commits into
mainfrom
codex/dashboard-ascend-356
Nov 5, 2026
Merged

jiangkuaixue123 merged 4 commits into
mainfrom
codex/dashboard-ascend-356

Conversation

@jiangkuaixue123

@jiangkuaixue123 jiangkuaixue123 commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Governing Issue and delivery slice

Refs #356

  • Accepted scope / delegation: User requested an Ascend dashboard fix, local redeployment, browser verification, and a repair PR.
  • What this PR delivers: Existing Ascend experiments can enter the native dashboard through explicit hash-bound external records. Native cards, independent per-tier parent/two-round/aggregate/target measurements, performance and quality verdicts, events, source references, summaries, and collector freshness are displayed. Empty campaigns are identified as disconnected; explicit empty stage declarations are distinguished from missing history.
  • What it does not deliver: An experiment executor, automatic discovery of private campaign directories, inferred optimization history, or campaign acceptance. External verdicts never enter the generic verified gain ranking.

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

Requirement Actual change Check / evidence and result Remaining gap
External Ascend evidence in native views New external record reader; candidate binding and source SHA-256 checks; native UI and plots Regression tests plus actual local browser inspection in English and Chinese Caller owns the semantic mapping and frozen-protocol validity
Separate performance / quality and tiers Independent absolute latency axes, parent / two rounds / midpoint / target gap, quality count and verdict Formal records and incomplete attempts displayed in local deployment No claim of final optimization acceptance
Ascend environment identity Native signature includes verified devices, CANN / firmware, torch_npu and vllm_ascend Missing and changed runtime identity tests External fingerprints remain caller-declared, not certified comparability
Honest missing and live states Empty campaign integration state; source collection timestamp/status; immutable record publishing contract Missing/corrupt/changed-source/running/refresh tests Unrecorded diagnosis/search history remains unrecorded
Reproducible external integration docs/dashboard-external-records.md and dashboard guide link Documented external-only candidate schema Private read-only collector and campaign artifacts remain local

Checks and dependencies

  • Dashboard regression: 100 passed, including Node-backed UI checks.
  • Lint: ruff check src tests tools passed. git diff --check passed.
  • Local deployment: Reloaded the committed source and verified the loopback dashboard in Chrome. Thirteen existing task/attempt records, separate per-tier measurements, quality results, current-state summary, and source timestamps were visible. No inference service or experiment was launched or modified.
  • Full-suite attempt: 3308 passed, 284 failed, 165 skipped. The initial environment selected Apple Git 2.39.5; snapshot tests require Git >=2.45. Retrying failures with existing Git 2.53 yielded 264 passed / 13 failed; using a resolved temporary directory then yielded 9 passed / 4 failed. The initial full run also overlapped template edits; the affected read-only-tree test passed on retry.
  • Remaining four failures reproduce on untouched base e843b9d: release publisher incomplete-state test; runtime non-UTF8 filename test; retained-report clean-environment test; snapshot case-alias test. Full-suite green is not claimed. Optional/platform skips remain.
  • Isolation: Changes were made in a separate worktree. Campaign data, deployment paths, machine identifiers and benchmark values are not committed.
  • Review: Review feedback received from amy-why-3459; fixes are ready for re-review. No independent approval is claimed.

Remaining work and Issue closure

  • Remaining Issue requirements / follow-up: Independent review and CI; broader collector/backend rollout and full historical narrative integration remain separate from this bounded repair.
  • Goal-acceptance owner or accepted closure delegation: Human issue owner; no closure delegation.
  • Goal ready for acceptance? Not claimed. This PR does not close the governing issue.

Review follow-up (head d2752da)

Addresses the six review points:

  1. Integration messages now use separate paragraph/span nodes, including the translated empty-stage declaration; no newline CSS dependency. Node regression checks English → Chinese → English with collection state and the stage note together.
  2. Two-round midpoint validation uses relative tolerance 1e-9 / absolute tolerance 1e-12, documented in the contract. Tests accept binary/serialization roundoff and reject material differences; caller values remain unchanged.
  3. External records without metrics remain in candidate/event views and are counted explicitly in the chart note (also translated). Tests cover both entirely unmeasured and mixed measured/unmeasured views.
  4. Parent/R1/R2/aggregate colors and positions are bound to roles for 0, 1 and 2 samples. Regression coverage confirms measurement(undefined) renders “Not recorded”.
  5. Searched all consumers of overview.comparison_state: gain eligibility, static chart, timeline and template. Charts/timeline only accept comparable; no separate CLI/report enum consumer was found. An empty-campaign snapshot test covers no_data, absent best gain/target gap, and both empty charts; the chart note now explicitly says no measurements recorded.
  6. Previous-head Ready CI completed successfully: run 42022183029, including full-check 3.10 and full-check 3.11. The four local baseline failures did not fail that CI run. CI for d2752da6 completed successfully. This result applies to that reviewed head, not the subsequent mixed-campaign fix.

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.

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>

Copy link
Copy Markdown
Collaborator

Review of the full PR diff: base e843b9d31cb86e1fd12d00890c89798b0bedd1fe .. head 09925fc05e8d437d091b9ecec5aa28449fe0f391 (first review, draft).

The overall design (hash-bound external records, verdicts shown but never fed into the generic gain ranking, explicit not_connected state) looks reasonable to me. A few points I'd like addressed or clarified before this is ready:

  1. Chinese mode for the integration note — src/inferrsi/data/dashboard/template.html L527-531: the note is built by concatenating the base sentence with the collection line and the stages_declared is explicitly empty ... sentence. translated() (L381) matches exact strings / anchored patterns, so once anything is appended the composite text won't match the dictionary and the note will likely stay in English in zh mode. The stages_declared ... sentence also has no entry in chinese. Could you render these as separate nodes (each passed through translation) and add the missing zh string? Also, the '\n' separator only shows as a line break if #integration-note has white-space: pre-line/pre-wrap; I couldn't see that on .panel-body — please confirm or use separate elements.
  2. Exact float equality for two-round aggregate — src/inferrsi/harness/dashboard_external.py L53: aggregate != sum(samples) / 2 rejects the whole record as corrupt if the caller's median differs by FP rounding (e.g. computed in another language or rounded when serialized). Suggest math.isclose(aggregate, sum(samples) / 2, rel_tol=1e-9, abs_tol=...) or a documented rounding rule in docs/dashboard-external-records.md, plus a test with a non-exactly-representable pair.
  3. External rows without metrics drop out of the chart — src/inferrsi/harness/dashboard.py L141-145: external keeps rows whose external.metrics is non-empty, while native keeps rows with no external. A valid external record with metrics: [] (allowed by the reader) ends up in neither list, so it disappears from the chart with no "not recorded" hint. Is that intended? If so, please surface it somewhere (e.g. chart note) and add a test.
  4. Chart label/colour indexing with < 2 samples — dashboard_external.py L152-153: colours are picked by position min(j, 3), so with one sample the aggregate is drawn in the Round-2 colour, and the template table (template.html, protocol measurements block) reads m.samples[0] / m.samples[1] unconditionally. Please key colour/label by role (parent / round i / aggregate) and confirm measurement(undefined) renders as "Not recorded".
  5. New comparison_state = "no_data" — src/inferrsi/harness/dashboard_data.py L889: previously zero signatures gave "comparable". The template handles the new value, but are there other consumers of overview.comparison_state (reports, CLI output, snapshot fixtures) that assume the old value set? Please confirm, or add a test covering the zero-candidate overview.
  6. Evidence — full-check (3.10) / full-check (3.11) were still in progress when I looked, and the PR notes 4 local failures that reproduce on base. Once CI finishes, please link the result; if any of those 4 also fail in CI, a pointer to the tracking issue would help reviewers.

Not approving yet; I'll re-review on the next push.

@jiangkuaixue123
jiangkuaixue123 marked this pull request as ready for review November 5, 2026 11:50

jiangkuaixue123 commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator Author

@david6666666 麻烦帮忙 review 一下这个 PR,谢谢!已转为 Ready,主要修复 Ascend 外部优化记录无法进入原生 Dashboard 的问题,包括分档双轮指标、独立性能/质量判定、技术事件和采集状态。相关回归测试 91 项通过;全仓剩余 4 项失败已在未修改基线上复现,详情见 PR 描述。

@Dong1017 Dong1017 added the area:workbench CLI workbench: profile, A/B loop, quality gates, evidence reports label Nov 5, 2026 — with ChatGPT Codex Connector

@Dong1017 Dong1017 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Copy Markdown
Collaborator

Re-review of incremental changes: 09925fc05e8d437d091b9ecec5aa28449fe0f391 .. d2752da6f28147891136340a524f5cbdb546b9a2 (commits a7ce9d07d161, d2752da6f281).

Thanks for the follow-up — most of my previous points look addressed on this head:

  1. Integration note i18n — renderIntegration now emits separate <p> nodes; the explicit-empty stages_declared string has a zh entry; translation regression covers the connected/stale/empty-declaration path.
  2. two_round_median float check — math.isclose(..., rel_tol=1e-9, abs_tol=1e-12) with docs + parameterized tests.
  3. Empty-metrics external rows — stay in the candidate set; chart note explains the count; covered by test_empty_external_metrics_remain_visible_and_are_explained.
  4. Chart colours/labels — role-keyed (Parent/R1/R2/Aggregate); measurement(undefined) → Not recorded asserted.
  5. comparison_state = "no_data" — _chart returns an empty chart with an explicit note; empty-campaign snapshot test added. (I did not audit non-dashboard consumers beyond this path.)
  6. CI — on this head, full-check (3.10/3.11), lint-and-smoke, installed-package, pr-references, kv-task-qualification, and declarations are green.

Still blocking for my approve (also called out by @Dong1017 on template.html around the overview figures):

  • Mixed native + external undercounts recorded performance failures. renderStats only swaps in the performance_verdict-based failure figure when external.length === rows.length (src/inferrsi/data/dashboard/template.html ~L549–562). In a mixed campaign, generic Failed / regressed still keys off row.outcome / decision. Because valid external rows are forced to outcome: unverified in dashboard_data.py, a native row plus an external performance_verdict: FAIL can show 0 failures. Please either (a) always count external performance_verdict failures (and optionally quality) separately from native outcome counts, or (b) scope the generic figure to non-external rows and surface external FAIL/PASS beside it. A focused regression with one native candidate and one external FAIL would lock the contract.

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

Copy link
Copy Markdown
Contributor

@zengchuang-hw review

@amy-why-3459 amy-why-3459 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_separately exercises the real renderStats body 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 zengchuang-hw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 renderStats regression 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.

@jiangkuaixue123
jiangkuaixue123 merged commit f1c90df into main Nov 5, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:workbench CLI workbench: profile, A/B loop, quality gates, evidence reports

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants