Repository navigation
feat(workflow-eval): add switchable scorer configs - #984
Abhijeet Prasad (AbhiPrasad) wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1462109850
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| raise ValueError(f"Unknown workflow scorer {scorer_name!r}") | ||
| if not isinstance(scorer, _SwitchableScorer): | ||
| raise ValueError(f"Scorer {scorer_name!r} does not support inference mode changes") | ||
| lease_key = self._key(run_id, "inference-mode-lease", scorer_name) |
There was a problem hiding this comment.
When two callers concurrently switch different scorers, their leases use different keys even though both perform a read-modify-write of the same run record. Both calls can therefore read the original modes and the later _write overwrites the other scorer's update, silently leaving one scorer in the wrong inference mode. Use a run-wide lease or an atomic update for the shared record.
Useful? React with 👍 / 👎.
| error = await self._read(run_id, _stage_kind("score-error", name), item_id) | ||
| if error is None: | ||
| complete = False | ||
| else: | ||
| await self._log_score_error(run, case, name, error) |
There was a problem hiding this comment.
During pending="race", a batch callback can persist score-error while another worker is still executing the instant scorer. This worker's _run_ordinary_score returns immediately because the instant worker already owns the durable score claim, after which this branch logs the batch error, marks the case complete, and can finalize the run without a score; the later successful instant result cannot replace the existing error log. An active competing attempt must keep the case pending rather than treating the batch error as final.
Useful? React with 👍 / 👎.
| if run.get("inference_modes", {}).get(name, scorer.mode) == "instant": | ||
| batch_link = await self._read_batch_link(run_id, f"score:{name}", item_id) | ||
| if batch_link is None or name in run.get("race_inference", []): | ||
| await self._run_ordinary_score(run, case, scorer.instant, name) |
There was a problem hiding this comment.
When instant is a scorer class, which EvalScorer explicitly permits and the runtime validation accepts, the outer _SwitchableScorer prevents the class-instantiation logic in _advance from seeing it. Passing scorer.instant here consequently invokes the class's unbound eval_async and fails because no instance supplies self. Instantiate class-based instant scorers just as ordinary entries in scores are instantiated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d69aa24194
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| race_scorers.append(scorer_name) | ||
| elif pending == "drain" and scorer_name in race_scorers: | ||
| race_scorers.remove(scorer_name) | ||
| await self._write(run_id, "run", run) |
There was a problem hiding this comment.
When set_inference_mode overlaps the final poll or webhook completion on Redis or another yielding store, it can read a running record, let _save_completed persist the completed record, and then overwrite it here with its stale running copy. The finalize claim has already been consumed, so subsequent _advance calls return a permanently waiting status and cannot restore the summary; protect both the mode read-modify-write and the completion transition with the same lease or an atomic compare-and-set.
Useful? React with 👍 / 👎.
| if run.get("inference_modes", {}).get(scorer.name, scorer.mode) == "batch": | ||
| await self._schedule_batch_stage(run, scorer.batch, kind="score") |
There was a problem hiding this comment.
When a concurrent poll or webhook handler loaded a batch-mode snapshot just before set_inference_mode(..., mode="instant") commits, it can still pass this stale condition afterward and irreversibly reserve and submit batch work. For pending="drain", previously unsubmitted items then either wait for this newly created batch or race it and incur both provider costs instead of using instant inference; coordinate the mode check with batch reservation or re-read it atomically immediately before scheduling.
Useful? React with 👍 / 👎.
| if isinstance(scorer, WorkflowBatchScorer): | ||
| if isinstance(scorer, _SwitchableScorer): | ||
| if run.get("inference_modes", {}).get(scorer.name, scorer.mode) == "batch": | ||
| await self._schedule_batch_stage(run, scorer.batch, kind="score") |
There was a problem hiding this comment.
When another controller switches an instant-mode scorer back to batch while its instant call is still active, this schedules every result-less and batch-link-less item without checking score-attempt-active. That starts a duplicate batch provider operation even though drain semantics require the existing instant call to finish, potentially charging for both attempts; omit active or already-claimed instant items from the batch candidates.
Useful? React with 👍 / 👎.
Switch judge inference while a workflow evaluation is running
Workflow evaluations often need two ways to run the same judge: batch inference is useful for throughput and cost, while regular inference is useful when results are needed sooner.
switchable_scoreslets an evaluation start in either mode and change modes for a specific run without changing the judge's scorer identity.The mapping key is the judge's canonical name. Use it for
WorkflowBatchScorer.nameand when callingset_inference_mode. Ordinary scorers stay inscores; switchable judges are configured separately inswitchable_scores.What happens when you switch
With
pending="drain", existing attempts continue and no competitor is started for a pending batch submission. Work without an attempt uses the selected mode. Withpending="race", unresolved items already submitted in batch also get an instant attempt. The first successful result accepted for each case/trial and judge is retained; late results do not replace it. Racing can run both providers and incur charges for both.Switching back to
mode="batch"withpending="drain"sends future unsubmitted judge work through the batch scorer while existing instant calls finish. Task inference is unaffected. The selected mode is stored per run and judge, so resuming with a fresh definition uses the run's persisted mode even if its initialmodediffers.When to use each approach
scoreswhen it should always use ordinary inference.WorkflowBatchScorerinscoreswhen it should always use batch inference.switchable_scoreswhen you want batch execution by default and the option to promote a run to instant inference, or when you want to race unresolved batch work for a faster result.