Skip to content

fix: apply reviewed maintenance repairs for #2930 - #3696

Closed
kovtcharov wants to merge 1 commit into
amd:ci/eval-drop-repeatsfrom
kovtcharov:codex/maintenance-pr-2930
Closed

kovtcharov wants to merge 1 commit into
amd:ci/eval-drop-repeatsfrom
kovtcharov:codex/maintenance-pr-2930

Conversation

@kovtcharov

Copy link
Copy Markdown
Contributor

Follow-up fixes for #2930. Changed: .github/workflows/email_scorecard_refresh.yml; hub/agents/email/python/packaging/assert_llm_coverage.py; hub/agents/email/python/tests/test_assert_llm_coverage.py; tests/unit/eval/test_email_refresh_timeouts.py.

This PR targets the original feature branch because the current account cannot push to amd/gaia. It carries the prepared maintenance changes for that PR.

Test plan

  • Previously completed local validation: Validation: 64 tests passed across refresh timeout script execution, scorecard gate, and coverage validation (tests-2930.log). Tests execute the actual workflow budget script for one/three experiments and full/subset profiles. Black/isort/Flake8/diff pass. Workflow YAML parses.
  • Fresh CI on the combined feature branch.
  • Remaining live-model, hardware or platform checks described in the original PR, where applicable.
  • Human review before merging the original PR.

Changed: .github/workflows/email_scorecard_refresh.yml; hub/agents/email/python/packaging/assert_llm_coverage.py; hub/agents/email/python/tests/test_assert_llm_coverage.py; tests/unit/eval/test_email_refresh_timeouts.py.
@github-actions github-actions Bot added devops DevOps/infrastructure changes tests Test changes agent::email Email agent changes sidecar Agent sidecar contract / harness labels Sep 11, 2026
@kovtcharov
kovtcharov marked this pull request as ready for review September 17, 2026 20:04
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions

This makes the email scorecard-refresh workflow size its own timeouts from the dispatch inputs, so a repeat run (experiments > 1) is no longer killed by a budget that only ever fit one run — and it hardens the LLM-coverage gate against a nonsense classification count. The shape is right and the new test is unusually good: it runs the workflow's real budget script rather than re-implementing the arithmetic.

Three things worth fixing before merge, none blocking:

  • The new budget script decides "full run vs smoke run" by re-checking the message-count threshold itself. The workflow already publishes that decision as one shared value, with a comment right above it saying every downstream consumer must read it rather than re-derive the threshold. Two copies of a number will eventually disagree — read the shared one.
  • Nothing caps the computed budget. A mistyped repeat count now authorizes a multi-day job on the single shared AMD eval machine, where the old fixed ceiling would have cut it off. The workflow's own header spends fifteen lines warning about exactly this slot contention, so the budget should refuse an unreasonable repeat count instead of honouring it.
  • The new "bad count" error says the value is bad but not what it was or where to look. The two errors immediately next to it in the same function both name the value, the file, and the next thing to check.

Real-world evidence

N/A — CI configuration and tests only; no evidence-bundle.md was produced, and the change touches no CLI, API, MCP, or Agent UI surface. The verdict rests on static review plus the new unit test, which I read but could not execute (the review sandbox had no working shell).

🔍 Technical details

🟡 Important

1. Re-derives the >=249 full/subset threshold (.github/workflows/email_scorecard_refresh.yml:209)

The workflow defines IS_FULL at line 172 with an explicit comment at 169–171: "Single source of truth for the full/subset split (#2094, Reflection A1/S1) — every downstream ternary and the pre step read THIS, never re-derive the >=249 threshold themselves." The new script re-derives it. IS_FULL is workflow-level env, so the step already has it:

          import os
          limit = int(os.environ["LIMIT"])
          experiments = int(os.environ["EXPERIMENTS"])
          if limit < 1 or experiments < 1:
              raise SystemExit("limit and experiments must be positive integers")
          benchmark = (210 if os.environ["IS_FULL"] == "true" else 45) * experiments
          with open(os.environ["GITHUB_OUTPUT"], "a", encoding="utf-8") as output:
              output.write(f"benchmark={benchmark}\njob={benchmark + 230}\n")

LIMIT then becomes unused for the branch but is still worth validating. (Line 401's GAIA_AGENT_TOOL_TIMEOUT ternary violates the same rule, but that is pre-existing and out of scope here.)

2. No upper bound on the derived budget (.github/workflows/email_scorecard_refresh.yml:207)

The guard rejects < 1 but accepts anything above it. experiments=10 yields benchmark=2100, job=2330 — ~39h holding the serial lemonade-eval slot, which the six sibling workflows sharing the [self-hosted, Windows, stx] label then queue behind (header, lines 76–91). The previous flat 440 was an accidental but real backstop; this removes it. Suggest failing the dispatch rather than honouring it:

          if limit < 1 or experiments < 1:
              raise SystemExit("limit and experiments must be positive integers")
          if experiments > 4:
              raise SystemExit(
                  f"experiments={experiments} would budget a multi-day hold on the "
                  "serial lemonade-eval slot; measured run-to-run stdev is 0.0023 "
                  "(cv 0.27%), so repeats above 4 buy nothing. Re-dispatch lower."
              )

Pick whatever ceiling matches intent — the point is that there be one.

🟢 Minor

3. New error isn't actionable (hub/agents/email/python/packaging/assert_llm_coverage.py:107)

CLAUDE.md's "Actionable errors name three things" (what failed / what to do / where to look) — and the two sibling branches at lines 88 and 112 all do. This one names neither the offending value nor the file. The summary line also drops the ❌ its two siblings use, so the job summary renders inconsistently.

        print(
            f"ERROR: `llm_classified_count` is {classified!r} in {scorecard_path} -- "
            f"not a finite number, so LLM coverage cannot be verified.\n"
            f"  The scorecard was hand-edited, or the harness aggregation wrote a "
            f"corrupt value; check the performance block in src/gaia/eval/benchmark.py.",
            file=sys.stderr,
        )
        _emit_summary(
            "- **LLM coverage**: ❌ invalid classification count — results void"
        )

The existing assertion ("finite number" in ...err) still passes.

4. Non-numeric input still throws a raw traceback (.github/workflows/email_scorecard_refresh.yml:205)

int(os.environ["LIMIT"]) on limit=abc raises ValueError with a traceback, one line above a SystemExit that exists precisely to avoid that. Not a regression (the old fromJSON also blew up), but the friendly path is right there:

          try:
              limit = int(os.environ["LIMIT"])
              experiments = int(os.environ["EXPERIMENTS"])
          except ValueError as exc:
              raise SystemExit(f"limit and experiments must be integers: {exc}") from exc

5. Stale comment (.github/workflows/email_scorecard_refresh.yml:398-400)

"Keep BOTH values BELOW the step's own timeout-minutes (210 full / 45 subset)" — those are now per-experiment multiplicands, not the step's timeout. Still true as a floor, but the parenthetical no longer describes what the step is set to.

Strengths

  • tests/unit/eval/test_email_refresh_timeouts.py executes the YAML-embedded script through a subprocess with the real env, instead of re-implementing the formula in the test — a re-implemented formula would pass while the workflow is broken. It also asserts the needs: wiring and both ${{ }} expressions, so a half-applied refactor fails the test.
  • The NaN/bool holes closed in the coverage gate are real, not theoretical: float("nan") <= 0 is False and float(True) is 1.0, so both used to sail through the gate and publish a card.
  • The 230-minute fixed-overhead split is correct — the three judge steps run fixed-size corpora once regardless of --experiments, so only the benchmark scales. Both derived values reproduce the previous 440/275 at experiments=1, which the test pins.

@kovtcharov-amd
kovtcharov-amd deleted the branch amd:ci/eval-drop-repeats September 18, 2026 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent::email Email agent changes devops DevOps/infrastructure changes sidecar Agent sidecar contract / harness tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants