Repository navigation
Conversation
_merge_requirements resolved strategy-level and per-call requirements by supersede: when the strategy had its own requirements they won outright and the per-call list was dropped, so a requirement the caller configured could go unenforced with no error. This contradicted both the method name and the sample() docstring, which described the two lists as merged. Union the two lists instead (deduplicated, order-preserving) so every requirement configured on the strategy or at the call site is enforced, and align the strategy class docstrings with the merge semantics. Closes generative-computing#1698 Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
This branch has not been deployed
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.
Pull Request
Issue
Fixes #1698
Description
SamplingStrategy._merge_requirementsresolved strategy-level and per-call requirements by supersede rather than merge: when the strategy had its ownrequirementsthey won outright and the per-call list was silently dropped (and when it had none, the per-call list was used). The two lists were never combined. This contradicted both the method name and thesample()requirementsdocstring, which have described the lists as "merged with global requirements" since sampling was first added — a quiet correctness footgun where a requirement the caller configured could go unenforced with no error or warning.This changes
_merge_requirementsto return the union of both lists (deduplicated, order-preserving), so every requirement configured on the strategy or at the call site is enforced. BecauseRequirementhas no value equality, deduplication is by object identity: two distinctRequirementinstances are always kept and validated, and a real duplicate only arises when the same instance appears in both lists (where which copy is kept is immaterial). The strategy class docstrings (SamplingStrategy,BaseSamplingStrategy,BaseMBRDSampling) are aligned with the merge semantics.Because this changes released behavior for
act()/aact()/instruct()/sample()that has shipped for ~13 months, it was split out of #1697 into its own issue (#1698) and reviewed before implementing.Testing
Inverted the existing
test_global_requirements_override_per_call(nowtest_global_and_per_call_requirements_are_merged) to assert both lists are evaluated, and addedtest_merged_requirements_dedup_shared_instancecovering identity-based deduplication of a requirement present in both lists.Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.