[ajbozarth]

Pull Request

Issue

Fixes #1698

Description

SamplingStrategy._merge_requirements resolved strategy-level and per-call requirements by supersede rather than merge: when the strategy had its own requirements they 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 the sample() requirements docstring, 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_requirements to return the union of both lists (deduplicated, order-preserving), so every requirement configured on the strategy or at the call site is enforced. Because Requirement has no value equality, deduplication is by object identity: two distinct Requirement instances 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

• Tests added to the respective file if code was changed
• New code has 100% coverage if code was added
• Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Inverted the existing test_global_requirements_override_per_call (now test_global_and_per_call_requirements_are_merged) to assert both lists are evaluated, and added test_merged_requirements_dedup_shared_instance covering identity-based deduplication of a requirement present in both lists.

Attribution

• AI coding assistants used

---

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.

• Component
• Requirement
• Sampling Strategy
• Tool

[jakelorocco]

There are also some examples that need to be updated if we make this change:

# docs/examples/mini_researcher/researcher.py:194 
    report_result = m.instruct(
        description="Context:\n{{context}}\nSummarize the relevant information available into a detailed report on how {{current_subtopic}} impacts {{main_topic}}.\n\nFollow this outline:\n{{outline}}",
        requirements=[req_focus, req_length, req_language, req_tone],
        user_variables=user_args,
        strategy=RejectionSamplingStrategy(loop_budget=2, requirements=[req_length]),
        return_sampling_results=True,
    )

# mellea/stdlib/sampling/feedback.py:360
class ModelFriendlyRepairStrategy(RepairTemplateStrategy):
    """RepairTemplateStrategy with model-friendly feedback formatting.

    Extends RepairTemplateStrategy to use ModelFriendlyFeedbackFormatter for
    converting validation failures into actionable repair guidance. This typically
    improves LLM performance on repair tasks compared to generic validation reasons.

    Example:
        >>> strategy = ModelFriendlyRepairStrategy(loop_budget=2, requirements=[...])
        >>> result = session.instruct(
        ...     "Write Python code",
        ...     requirements=[...],
        ...     strategy=strategy,
        ... )
    """

# mellea/stdlib/sampling/presets.py:145
    return SamplingPreset(
        requirements=requirements,
        strategy=strategy,
        feedback_strategy_name="python_code_repair",
        description="Python code generation with smart repair feedback",
        example_usage=(
            "preset = python_code_generation_sampling(allowed_imports=['numpy'])\n"
            "result = session.instruct(prompt, requirements=preset.requirements, strategy=preset.strategy)"
        ),
    )

[jakelorocco]

I don't know if we care (since they are technically different requirements with different wording) but this only treats two requirements as the same if they are the same object. When user_variables is set, Instruction.init makes a deep copy of every Requirement. So a requirement passed to both the strategy and instruct() ends up as two objects and is validated twice.

[ajbozarth]

Upon investigation into the code I think the behavior is fine, I plan to better document that a req shouldn't be put on both the Strat and the caller function. I'll make that doc addition/update alongside the example fixes and a few other items I found I missed in my next commit (currently tracking down a potential missed code path that mellea/stdlib/sampling/presets.py:145 lead me to).

[ajbozarth]

after iterating on it locally I ended up not adding any explicit doc updates, but did update the examples you noted to demo the updated correct behavior. fixed in a90d070

[jakelorocco]

After reading through the code again, I think I'm having second thoughts on this:

1. I'm not sure merging is the correct call. When we talked, we had mentioned that the requirements from the most recent call would be the ones utilized, not a merging. I think merging makes sense, but then we should actually do it so that all sampling strategies benefit from it?
2. Perhaps we ought to have a flag that can be passed to instruct() that says whether the included requirements should be checked or if they are just to be included in the text of the instruction? Or maybe this can be a flag on the requirement to make no-op requirements easier?

[ajbozarth]

I just want to make sure I parsed you feedback here correctly: You had doubt that merge is correct like we though then talked yourself back into it still being correct, then pitched the idea of potentially putting merge or not merge behind a feature flag for user control?

Assuming I understood that correctly I would have to disagree with the feature flag idea. I think better documentation of the merge behavior as I intend to add (see #1744 (comment)) would suffice as user control, as they should have control over both the strategy and caller and which requirements those set.

If I misunderstood you please clarify exactly what your feedback is.