Skip to content

fix(registry): round-trip parameterized score reducers#4519

Open
jstar0 wants to merge 2 commits into
UKGovernmentBEIS:mainfrom
jstar0:fix/reducer-registry-roundtrip
Open

fix(registry): round-trip parameterized score reducers#4519
jstar0 wants to merge 2 commits into
UKGovernmentBEIS:mainfrom
jstar0:fix/reducer-registry-roundtrip

Conversation

@jstar0

@jstar0 jstar0 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

This PR contains:

  • New features
  • Changes to dev-tools e.g. CI config / github tooling
  • Docs
  • Bug fixes
  • Code refactor

What is the current behavior? (You can also link to an open issue here)

Part of #4374.

Directly constructed parameterized score reducers such as at_least(2), pass_at(2), and pass_k(2) serialize their human-facing names (at_least_2, etc.) into RegistryDict.name. Those suffixed names are not registered factory keys, so replay through registry_arg(registry_value(reducer)) raises LookupError even though k is already present in the serialized params.

What is the new behavior?

Score reducer instances keep the registered factory name as their registry identity. Parameterized reducers now serialize with names such as at_least plus {"k": 2}, so they restore through the registered factory.

Human-facing reducer names remain unchanged: reducer_log_name() still reports at_least_2, pass_at_2, and pass_k_2 from the restored params.

Regression coverage exercises direct serialization and replay for all three affected reducers and preserves the existing custom numeric-name behavior.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No user-facing breaking change is expected. Parameterized reducer log names remain unchanged; only their internal registry identity is corrected to use a replayable factory key.

Other information:

Verification:

  • uv run pytest -q tests/util/test_registry.py tests/scorer/test_reducers.py tests/test_task_source.py (70 passed, 2 skipped)
  • uv run make check
  • Full uv run make test: 7752 passed, 3203 skipped; the two remaining Anthropic default-header failures reproduce on the unchanged base revision.

@dragonstyle dragonstyle self-assigned this Jul 17, 2026

@ebarkhordar ebarkhordar left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix, and I checked it does what it says: installed the branch at 0d8d8f9 and confirmed at_least(3) now registers as at_least with params={"k": 3}, so registry_arg(registry_value(at_least(3))) round-trips and reducer_log_name still returns at_least_3. The old name="at_least_3" had no matching factory, so a restore couldn't have worked before.

One thing this opens up: the change removes the only reader of REDUCER_NAME. wrapper no longer calls getattr(score_reducer, REDUCER_NAME, ...), and reducer_log_name builds the _k suffix from registry_params, not from the attribute. But at_least/pass_at/pass_k still setattr(reduce, REDUCER_NAME, ...) at reducer.py:116, :162, and :202, so the attribute is now written and never read in src/. grep -rn __REDUCER_NAME__ src/ shows only the constant, the import, and those three writes.

Is REDUCER_NAME meant to stay (read by something downstream I missed), or can the three setattr calls plus the constant and import go? Asking because the new getattr(r3, REDUCER_NAME) == "at_least_3" assertions pin an attribute nothing consumes, which could read as load-bearing to the next person in this file.

@jstar0
jstar0 force-pushed the fix/reducer-registry-roundtrip branch from 0d8d8f9 to 5f4bde6 Compare July 18, 2026 11:11
@jstar0

jstar0 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

You're right. REDUCER_NAME only supported the old registry-name override. Now that log names are derived from the stable factory name and registry_params, the attribute had no remaining reader.

I removed the constant and import, all three setattr writes, and the assertions that pinned that unused mechanism in 5f4bde60. The behavioral coverage remains: registry names stay stable and reducer_log_name() still returns the parameterized names.

Verification after the cleanup:

  • uv run pytest -q tests/util/test_registry.py tests/scorer/test_reducers.py tests/test_task_source.py (70 passed, 2 skipped)
  • uv run make check

Thanks for catching this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants