Wait for the requested scorecard calculation before finishing an eval - #334
Open
CTWalk wants to merge 1 commit into
Open
Wait for the requested scorecard calculation before finishing an eval#334CTWalk wants to merge 1 commit into
CTWalk wants to merge 1 commit into
Conversation
_scorecard_is_done() ignored a mismatched latest_calculation.id only while the payload was non-terminal. A previous calculation is normally terminal, so an eventually consistent first read could satisfy the wait after one poll. The returned id is also reused as the calculation_id parameter when fetching row scores, so a stale-accepted poll produced an aggregate and row-level scorer outputs that both belonged to the previous calculation. Treat any mismatched latest calculation as stale regardless of status, and extend the existing polling test with an old completed calculation ahead of the requested run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for the eval runner in #333 — it was a pleasure to read, and this is a
small correctness fix in that new code rather than a complaint about it. Happy
to close it if you already have this in flight.
Summary
ID, even when that previous calculation is already terminal.
the requested run.
Problem
recalculate_and_wait_scorecard()starts a calculation, receives its ID, thenpolls the sheet scorecard endpoint.
_scorecard_is_done()currently ignores amismatched ID only while the response is non-terminal:
A previous calculation is normally terminal, so if the first read still exposes
it, the terminal exception accepts it immediately — after a single poll. The
comment above the guard describes the intent ("ignore stale payloads from a
previous calculation until the new one appears"), but the terminal clause
defeats it for the most ordinary stale payload: a completed previous run.
Both
recalculate_and_wait_scorecard()andarecalculate_and_wait_scorecard()route through this predicate.Why it is worth fixing
The mismatched ID is not only compared, it is reused. After polling,
extract_scorecard_scores()readslatest_calculation.idback out of thepayload and sends it to the row endpoint as
calculation_id(
evaluations/scorecard.pyL290-291 and L322-323):So a stale-accepted poll yields an aggregate score and row-level scorer outputs
that agree with each other and both belong to the previous calculation. The
result is a plausible wrong report rather than a visibly broken one: a previous
green run can stand in for a new regression, or a previous red run can fail a
new healthy run.
That reuse is also why I think the ID comparison is meant to be authoritative
rather than advisory — the SDK already treats the recalculate ID and
latest_calculation.idas the same namespace.Behavior after this change
A response carrying another calculation ID is treated as stale regardless of
status. Polling completes only once the requested calculation is visible and
terminal.
The guard stays conditional on the API returning a latest calculation ID, so the
existing fallback for payloads without one is unchanged.
Reproduce
The updated regression test supplies three responses:
calc-old, completed;calc-1, running; andcalc-1, completed.Before the change the poller returns the first response after one read. After
it, the poller makes all three reads and returns the requested completed
calculation. Reverting only the
scorecard.pychange makes the test fail oncalc-old != calc-1, so it pins the behavior rather than restating it.Verification
Run in the environment
integration-tests.ymldeclares — Python 3.11 (3.11.14),Poetry 2.2.1,
poetry installfrom the committed lock:pytest -q— 357 passed, with the 13 pre-existing Pydantic deprecationwarnings
ruff checkandruff check --select Ion both changed files — cleanruff format --checkon both changed files — already formatted(ruff 0.9.4, the
.pre-commit-config.yamlpin)git diff --checkNo API key, model request, or token spend is involved in the test path.
One question for you
If the backend can legitimately report a different latest calculation while
the requested one is still queued, this change converts today's silent wrong
result into the existing
Timed out waiting for scorecard calculation to finish.error. I think failing loudly is the right trade for evaluationresults, but you know the backend semantics and I don't — if you would rather
bound the wait (accept a mismatched terminal payload only after N polls, or key
completion off a different authoritative field), tell me which and I will
rework it.
Cross-SDK note
prompt-layer-jshas the same terminal-ID exception insrc/evaluations/scorecard.ts, and reuses the polledlatest.idas the rowcalculation_idin the same way. I kept this PR scoped to the Python SDK anddid not run the JavaScript suite; glad to mirror the fix and the regression
there if you would prefer a paired change.
🤖 Generated with Claude Code