Repository navigation
Qualify RSI trials with explicit completeness and lifecycle checks - #341
amy-why-3459 wants to merge 5 commits into
Conversation
Add optional per-trial qualification without changing default command behavior. Document Omni reference, streaming, graph and lifecycle evidence contracts. Agent-assisted-by: Codex (implementation) Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
Regenerate all catalog provenance from one committed source snapshot as required by the index contract. Agent-assisted-by: Codex (implementation) Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
Skill evaluationPLANNED: not executed; there are no task outcomes yet.
Author verification steps (nonpaid; not yet run)
python tools/build_skill_index.py --source-revision 9e6a51cab505a8ae17575dc83ea53fc0d3b8b027 --checkThe declared source pin matches this head's canonical library bytes. The command has not run: retain its exit code and full drift/success output. It checks the generated projection, not claim truth or skill efficacy. Evidence and logs · Retention: 30 days. Plan bindings, budget, frozen exam and advisory detailsAdvisory: obtain missing evidenceRetained evidence availability: planned / unexecuted. No approval or merge authorization is issued.
Model/settings:
Automatic execution / cumulative PR budget: {"reason":"Trusted configuration or pricing missing, invalid or expired","remaining_usd":"unknown","reserved_or_unresolved_usd":"unknown","spent_or_legacy_usd":"unknown","state":"blocked"}Projection check selection (not an executed result): {"declared_in":"src/inferrsi/data/repo_packs/inferrsi/knowledge/README.md","source_revision":"9e6a51cab505a8ae17575dc83ea53fc0d3b8b027","status":"ready"}Provider/settings: Uncovered / unsupported paths (96)
|
Agent-assisted-by: Codex (implementation) Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
Dong1017
added
the
area:workbench
ChatGPT Codex Connector
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
Scoped static review of draft #341 at head 8d01acd0 against Issue #340: inspected CLI/RunSpec freezing, per-trial result collection, evaluation precedence, the synthetic tests, and the existing HTML/SVG report consumer. One P2 evidence-presentation risk is attached inline: unqualified trials correctly suppress improvement_pct, but the report still plots an apparent gain ratio from their raw medians. Please make the unqualified status unmistakable in that consumer. This does not challenge retention of raw measurements or the declared adapter-attestation boundary.
I did not run tests, model workloads, or hardware experiments. The separate current-head knowledge-authorship admission failure remains an author/maintainer matter; this COMMENT is neither approval nor integration/goal acceptance.
| t.get("status") == "completed" and t.get("metric") is not None for t in baseline + candidate | ||
| ) | ||
| if all_metrics: | ||
| if all_metrics and trials_qualified: |
There was a problem hiding this comment.
P2 — unqualified trial checks still produce a comparative gain chart. With baseline=100 ms, candidate=80 ms and evidence.workers_closed=false, this branch correctly leaves improvement_pct=None and yields INCONCLUSIVE. However harness/reporting.py::_summary_figure() calls summary_chart.py::spec_from_evaluation(), which derives a candidate/baseline ratio directly from the retained medians and renders it in report.html even when checks failed. The point note says INCONCLUSIVE, but the chart still visually communicates an improvement that this new gate is meant to withhold. Consider suppressing the gain figure for an unqualified comparison, or explicitly marking the plotted ratio as raw/unqualified — not an accepted gain. Please add a regression that reads the HTML/SVG report after a false/missing required check; the existing tests only assert evaluation.json.
There was a problem hiding this comment.
Addressed in 8970059. evaluate() now records trials_qualified, and spec_from_evaluation() refuses to derive a candidate/baseline ratio when it is false. For an unqualified comparison, report.html labels the retained medians as raw measurements (not an accepted gain) and no summary-chart.svg is written; qualified comparisons still plot the measured ratio. Added regressions that read the HTML report for both a false and a missing evidence.workers_closed check (baseline 100 ms / candidate 80 ms), asserting no SVG and no 0.80×. ruff and pytest tests/test_trial_checks.py tests/test_summary_chart.py tests/test_runtime.py pass locally; CI pending.
There was a problem hiding this comment.
Follow-up on the 8970059 repair (static review only): the single-run path now guards trials_qualified=false, but harness/reporting.py:69–75 prioritizes a pre-existing summary-chart.json and never calls the guarded spec_from_evaluation() in that branch. With a valid campaign spec alongside an unqualified current evaluation, _summary_figure() still writes and inlines an SVG, so “no gain figure on unqualified checks” is not guaranteed for this path. Please clarify whether the campaign figure is intentionally independent historical evidence; if so, label that boundary explicitly and ensure it cannot represent the unqualified current step as an accepted gain. Otherwise apply the qualification guard before either chart path. A regression using summary-chart.json plus trials_qualified=false would distinguish the intended behavior. No tests or model/device experiments run here.
There was a problem hiding this comment.
Fixed in 1cdbb70: when the current evaluation has trials_qualified=false, the report now stops before both chart paths, so no gain chart is written or inlined, even if a summary-chart.json exists. The spec file stays on disk untouched and the report notes it as separate historical evidence that was not drawn. Regression test: a summary-chart.json that would render 0.80× plus trials_qualified=false produces no chart and leaves the spec bytes unchanged.
A false or missing required check already leaves improvement_pct unset and the verdict INCONCLUSIVE, but the HTML report still plotted a candidate/baseline ratio from the retained medians. Refuse that derived figure and say the medians are raw measurements, not an accepted gain. Addresses #341 (comment) Agent-assisted-by: Cursor Grok 4.7 (implementation) Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: amy-why-3459 <amy-why-3459@users.noreply.github.com>
summary-chart.json is historical campaign evidence and still wins for a qualified run, but an unqualified evaluation was inlining that figure as this step's gain summary. Guard both chart paths before rendering, leave the committed specification unchanged, and say it was not drawn for this step. Addresses #341 (comment) Agent-assisted-by: Cursor Grok 4.7 (implementation) Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: amy-why-3459 <amy-why-3459@users.noreply.github.com>
Governing Issue and delivery slice
Refs #340
The contributor requested implementation and PR submission of the Omni integration workflow improvements. This delivers a bounded harness change plus portable workflow guidance. Merge and goal acceptance remain with the owner/maintainers.
Problem and resulting behavior
A benchmark can exit successfully and emit a faster metric while lacking evidence that all requests finished, stage workers exited normally, or the intended graph controls were used. The external-command harness previously had no way to require these adapter-produced qualifications on every trial.
Repeatable
--require-trial-check DOTTED_KEYoptions now freeze selected checks in RunSpec. Both arms must emit literaltruefor every selected key on every measured trial. Missing, unreadable, non-boolean or false checks yieldINCONCLUSIVEand noimprovement_pct; raw metrics and selected checks remain in the artifacts. Checks are optional for backward compatibility and do not replace explicit quality validation or source-integrity gates. They attest to adapter-owned evidence; the harness does not independently inspect detached workers or accelerator graph state.The packaged RSI evidence workflow now covers reference initialization/RNG contracts, feedback-aware stage boundaries, matching full/stream reference decoding, internal graph controls, streaming protocol preflight, device qualification, and complete-comparison reruns after harness repair. Quickstart and contributing guidance link to it. Campaign thresholds and hardware constants remain outside reusable defaults; no CI lane changes.
Changed artifacts and evidence
Checks and limitations
ruff check src tests tools, skill structure validation, relative links andgit diff --check: passed.tests/check_installed.py: passed.test_outer_worker_loss_detects_and_recovers_independent_session_survivors[timeout/cancel]) also fail on unchanged base663f3b327998526a13aa31e72ad729025c18983c(2 failed, 7 passed in that baseline file). They are not counted as passes.docs/skill-coverage.md; regenerated it in the final commit. Post-fix coverage-view, qualification and runtime verification: 79 passed, 15 subtests passed. Only the generated coverage document changed after the full run; the full suite was not repeated.No NPU/GPU or model-quality run is claimed: this change qualifies framework evidence and documents adapter responsibilities. It does not implement an Omni benchmark adapter, automatically reserve hardware, or turn boolean fields into independent hardware proof.
Remaining work and Issue closure
The bounded implementation is submitted for maintainer review and owner acceptance. No automatic Issue closure or campaign PASS is requested. Applying these checks to a real campaign requires frozen adapter validators and retained request/worker/graph receipts.
Knowledge authorship
Exact session model/effort settings are awaiting contributor confirmation; unknown values are intentionally retained rather than inferred.