refactor(tasks): share auxiliary model-role resolution on Task - #97
Merged
Conversation
Eight tasks each carried their own `_resolve_grader` / `_resolve_extractor`, seven of them byte-identical once the role name is normalised away. The declaration half of the contract was already shared as `Task._bind_role_requirement`; only the resolution half was copied, so the `models_by_role` protocol — pooled role model, both-supplied rejection, missing-role message — had eight owners that had to move together. Add `Task._resolve_role_model(role, configured, models_by_role, build=...)` and route all eight through it. `build` is a thunk so each task keeps its own constructor and its own "you must supply one" message, which is the part that legitimately differs: AGIEval's extractor needs the candidate model and admits the `self` sentinel, and each grader task explains its own scoring dependency. Behaviour is unchanged; every error message is byte-identical to the copy it replaces. Verified by mutation: removing the both-supplied guard fails 8 tests (one per task), making the helper ignore `models_by_role` fails 24, and mis-keying the role lookup fails 8 — so all eight tasks genuinely exercise the shared path rather than passing on their own leftovers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-ups from the review of this PR. `_resolve_role_model` takes a type parameter, so `configured` is typed by whatever `build` accepts rather than `object`, and `build` receives it instead of the call site closing over the same value a second time. The seven grader tasks pass `self._build_grader` directly — the empty lambda is gone — and AGIEval wraps it only to add the candidate model. Threading the value twice was how a mistyped `configured` could slip past the both-supplied guard and let a pooled model win silently; ty now rejects a mismatched pairing. Docstring cut to the two things the code does not already say. Both halves of the role contract get direct unit tests. The declaration half, `_bind_role_requirement`, had none: its missing-binding branch is unreachable from `model_requirements_for`, which picks the role out of the bindings it was handed, so the message and its a/an agreement were unasserted even though all eight tasks name a literal role and can miss. Behaviour is unchanged — all 48 supply combinations across the eight tasks are byte-identical to the pre-refactor code. The resolution mutations now fail 9, 27 and 9 tests (up from 8, 24 and 8) as the core tests catch each break independently. Five of six mutations to the declaration half are caught by the intended test; the sixth is a `requires` type guard that `TaskModelRequirement.__post_init__` already enforces with the same message, so no test can pin it as the sole enforcer — recorded in that test rather than papered over. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`requires must be TaskRequirements` was raised from three places with the same wording: `TaskModelRequirement.__post_init__`, which is the one with a direct test, plus copies in `_bind_model_requirements` and `_bind_role_requirement`. Both copies sit in front of a construction that re-checks the same thing, so they bought nothing but a third place for the wording to drift. Removed. The two class-named variants stay — `meta.py` at decoration and `_validate_model_requirements` at construction validate the `requires` ClassVar, a different contract, and their messages name the class. Not a pure no-op, so stating it: with the copies gone, a call that is invalid in *two* ways at once reports the binding problem instead of the type problem — `_bind_role_requirement(ctx, "grader", <not TaskRequirements>)` with no `grader` binding now raises the missing-binding ValueError, and `model_requirements_for` with a bad `requires` ClassVar *and* no bindings raises the no-candidate ValueError. Every singly-invalid call is unchanged, including a bad `requires` with a resolvable binding, which still raises the identical TypeError from __post_init__. Neither corner is reachable in production: `requires` always arrives as `cls.requires`, already rejected earlier by the two class-named guards. Co-Authored-By: Claude Opus 5 (1M context) <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.
Type
Summary
_resolve_grader/_resolve_extractor. Normalising the role name away, seven are byte-identical; AGIEval's differs only because its builder also needs the candidate model.Task._bind_role_requirement); only the resolution half was copied. So themodels_by_roleprotocol — pooled role model, both-supplied rejection, missing-role message — had eight owners that must move together, and the next grader-bearing benchmark would have added a ninth.Task._resolve_role_model(role, configured, models_by_role, build=...)and routes all eight through it.buildtakesconfiguredand returns the Model, so each task keeps its own constructor and its own "you must supply one" message — the part that legitimately differs (AGIEval admits theselfsentinel and explains its 26-point extractor spread; each grader task explains its own scoring dependency). The seven grader tasks passself._build_graderdirectly; AGIEval wraps it only to add the candidate model.configured: Tagainstbuild: Callable[[T], Model]— soconfiguredis typed by whatever the builder accepts rather thanobject, and the value is not threaded through the call twice. Threading it twice was how a mistypedconfiguredcould slip past the both-supplied guard and let a pooled model win silently;tynow rejects a mismatched pairing._bind_role_requirementhad none, and its missing-binding branch is unreachable frommodel_requirements_for(which picks the role out of the bindings it was handed), so that message and itsa/anagreement were entirely unasserted — even though all eight tasks name a literal role and can miss.requires must be TaskRequirements:TaskModelRequirement.__post_init__owns it, and the copies in_bind_model_requirements/_bind_role_requirementeach sat in front of a construction that re-checks the same thing. The two class-named variants stay — they validate therequiresClassVar, a different contract.The one behaviour exception
Everything about the shared helper is byte-identical to the code it replaces. The exception is the retired type check: a call invalid in two ways at once now reports the binding problem instead of the type problem —
_bind_role_requirement(ctx, "grader", <not TaskRequirements>)with nograderbinding raises the missing-bindingValueError, andmodel_requirements_forwith a badrequiresClassVar and no bindings raises the no-candidateValueError.Every singly-invalid call is unchanged, including a bad
requireswith a resolvable binding, which still raises the identicalTypeErrorfrom__post_init__. Neither corner is reachable in production:requiresalways arrives ascls.requires, already rejected earlier by the two class-named guards (sieval/core/tasks/meta.pyat decoration,_validate_model_requirementsat construction).Related Issues
Refs #25 — follow-up from the PR #45 review.
Test Plan
Automated
ruff check && ruff format --check)ty check)pdm run pytest) — 5186 passed; preflight 25 PASS, no FAIL/WARN;sync_package_stubs.py --checkandsync_meta_index.py --checkboth cleanManual
Behaviour pinned against the pre-refactor code. All eight tasks × six supply combinations (both supplied / missing role / pooled present / wrong role key / direct / neither) driven through both trees and diffed: 48/48 rows identical, exception chaining included. Both resolution entry points are classmethods, so this needs no dataset or model fixtures.
Mutation-tested the shared helper, because a green suite after a move refactor proves only that nothing imports the dead code. Three mutations against
Task._resolve_role_model, all caught:models_by_roleThe per-task failure counts are the point: all eight tasks genuinely execute the shared path, rather than passing on leftovers of their own.
Mutation-tested the declaration half, now that it has tests. Dropping the vowel branch from the
a/anarticle, lettingsource_taskinherit a parent's meta, dropping the role guard, dropping the context guard, and collapsingsource_taskto the bare class name are 5/5 caught, each by the intended test.Verified no lingering references to the removed methods anywhere in the tree, including
.pyistubs.Checklist
Required (all PRs)
type(scope): description)AI-Generated Code - <model> (<provider>)in module docstring — no new modules; all touched files already carry theirscore/— the helper lives onTaskincore/tasks/and imports nothing new beyondcollections.abc.Callable