Fix validate_choice substring check with swapped operands#1615
Fix validate_choice substring check with swapped operands#1615Chessing234 wants to merge 1 commit into
Conversation
validate_choice enforces the IFEval constraint 'Answer with one of the
following options: {options}'. The check
any(text in option for option in options)
tests whether the model's response is a substring of an option. Options
are short phrases (e.g. 'yes', 'red') and responses typically contain
extra words, so the common case 'I choose red' vs option 'red' returns
False, and only returns True when the response is itself contained in
an option — the inverse of the intended semantics.
Swap the operands to check whether an option appears in the response,
which matches the constraint's intent and the other validate_*
membership checks in this module.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the validate_choice function in open_instruct/if_functions.py to correctly check if any of the provided options are present within the input text. The review feedback suggests enhancing this check by making it case-insensitive to prevent false negatives due to capitalization differences and to ensure consistency with other validation functions in the module.
|
@Chessing234 thanks again for all these fixes! Please add a changelog item and I'm happy to merge :) |
…ai#1683) * Bundle small correctness fixes from allenai#1615/allenai#1618/allenai#1619/allenai#1623/allenai#1625/allenai#1646/allenai#1651/allenai#1655 with regression tests Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address PR review: inline write_header, pricing per 1M tokens, parameterize tests, dedupe if_functions Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Trim branch to IFEval-only changes; other fixes moved to allenai#1684/allenai#1685/allenai#1686 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address review: use word-boundary regex in validate_choice to avoid substring false positives Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Revert deletion of scripts/eval_constraints/if_functions.py (handled in separate PR) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Revert validate_choice regex change; add TODO comment with suggested word-boundary fix Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Update if_functions.py
|
Thanks for reviewing! As discussed over in PR #1651, @finbarrtimbers actually went ahead and bundled this fix along with my other recent PRs into a single PR (#1683) which included the changelog entry and tests. Since that one was successfully merged, this PR can be safely closed as superseded. |
|
Added a CHANGELOG entry! Thanks for reviewing and sorry for the delay. Happy to merge. |
3 participants
Bug
`validate_choice` enforces the IFEval constraint "Answer with one of the following options: {options}" (comment on the line above). The implementation:
```python
def validate_choice(text: str, options: list) -> bool:
return any(text in option for option in options)
```
Root cause
`text in option` tests whether the full model response is a substring of an option. Options are short phrases (e.g. `"yes"`, `"red"`) while responses are typically longer sentences, so the common case `validate_choice("I choose red", ["red", "blue"])` returns `False` — only responses that are themselves contained in an option pass, the inverse of the constraint's intent.
Other `validate_*` checks in the same module (e.g. `verify_keywords`) consistently look for the constraint tokens inside the response, not vice versa.
Fix
Swap the operands to `option in text`, matching both the constraint semantics and the rest of the module.
(The legacy copy in `scripts/eval_constraints/if_functions.py` has the same bug; I've left it alone to keep this change minimal since that file is not imported by the training code paths.)