fix(dspy): correct Refine/BestOfN fail_count budget accounting

[detail-app]

Warning

GitHub issue creation failed

Detail attempted to publish this bug to GitHub, but the issue could not be created. This fix PR was created without that issue, and missing tracker references are shown as Unknown issue.

You can review and merge this PR normally. Please review your tracker integration settings before the next publish run.

Detail bug report: View on Detail

📝 Changes Description

dspy.Refine and dspy.BestOfN accept a fail_count parameter documented as "The number of times the module can fail before raising an error". The implementation in forward() did not honor that contract:

• The except block compared the rollout index idx (which increments on every rollout, success or failure) against self.fail_count, instead of the number of failures that actually occurred. A single transient failure on a late rollout (after several successes) was re-raised even when the actual failure count was still within budget — e.g. with fail_count=2, one failure at idx=4 tripped 4 > 2 and raised.
• self.fail_count -= 1 mutated the instance attribute, so the failure budget leaked across forward() calls. After one call tolerated failures, the next call on the same instance started with a depleted (or negative) budget and silently tolerated fewer failures.

The bug was introduced in stanfordnlp#7926 (commit ef32f66d) and went undetected because the accompanying tests only covered failures starting at idx == 0, where idx and the failure count advance in lockstep.

Fix (dspy/predict/refine.py, dspy/predict/best_of_n.py): take a local fail_count = self.fail_count at the top of forward() (reset per call) and replace the except check with fail_count -= 1; if fail_count < 0: raise e. This compares actual failures to the budget and never mutates self.fail_count, fixing both the index-vs-count confusion and the cross-call leak in one change. The default fail_count or N now tolerates all N rollouts failing (returns the best prediction seen so far, None) rather than raising, matching the pre-stanfordnlp#7926 swallow-on-default behavior and the docstring.

Closes Unknown issue.

✅ Contributor Checklist

• Pre-Commit checks are passing (locally and remotely) — pre-commit run --files <changed files> exits 0 (ruff lint + check hooks)
• Title of your PR / MR corresponds to the required format — fix(dspy): correct Refine/BestOfN fail_count budget accounting
• Commit message follows required format {label}(dspy): {message} — fix(dspy): correct Refine/BestOfN fail_count budget accounting

⚠️ Warnings

• The existing test_refine_module_default_fail_count / test_refine_module_default_fail_count (best_of_n) tests encoded the buggy off-by-one behavior (asserting that fail_count=N with an always-failing module raises). They are updated to assert the corrected semantics: with the default, all N failures are tolerated and the module returns None. If the intended contract is instead "raise once failures exceed fail_count" even at the default, the default should be set to fail_count=N+1 rather than reverting the accounting logic — flagging here so that decision is explicit.
• This change is AI-authored by Detail under direct user guidance (AI-assisted contribution). The root cause was reproduced from a failing test before the fix, and all verification was run locally; no autonomous PR submission is intended — the human user reviewed and directed each step.

Testing

• New regression tests (in tests/predict/test_refine.py and tests/predict/test_best_of_n.py): a single late failure is tolerated with fail_count=2; a second late failure with fail_count=1 still re-raises; and self.fail_count is unchanged after one or two forward() calls (no cross-call leak). The updated default-fail_count tests assert the corrected None-return semantics and that self.fail_count is not mutated.
• The 8 new/updated regression tests were confirmed to fail on the un-fixed source (via git stash of the source fix) and pass on the fixed source, so they genuinely encode the bug rather than passing trivially.
• Routine checks: uv run ruff check and uv run ruff format --check are clean on the changed files; uv run pytest tests/predict/test_refine.py tests/predict/test_best_of_n.py -v → 12 passed; the broader uv run pytest tests/predict -m 'not extra and not deno' → 252 passed, 2 skipped, 0 failed; the full non-live suite (uv run pytest -m 'not extra and not deno' -n auto --dist worksteal tests/) → 1256 passed, 0 failed. No type checker is configured for this repo.
• End-to-end smoke against a live LM: no LM provider key was available in the environment, so a local Ollama server was started (ollama serve with qwen3:0.6b pulled) and LM_FOR_TEST=ollama/qwen3:0.6b uv run pytest -m llm_call --llm_call tests/ was run → 5 passed, 19 skipped (provider-specific test_lm_direct_live.py tests that require openai/anthropic/gemini backends), 0 failed. Note: no in-tree llm_call test covers Refine/BestOfN directly, so this is a defense-in-depth check of the LM-calling stack rather than direct coverage of this fix; the fix is pure control-flow logic and is backend-agnostic.
• Whole-repo ruff format --check reports 109 pre-existing files needing reformatting on the un-modified baseline (identical count with the fix stashed), so this change introduces no new formatting debt; ruff format is not a CI gate here.

---

Automatic Fixes PRs can be configured here.

[greptile-apps]

Greptile Summary

This PR corrects failure-budget accounting in BestOfN and Refine.

• Counts caught exceptions rather than relying on rollout position.
• Resets the available failure budget for every invocation without mutating module state.
• Adds regression coverage for late failures, exhausted budgets, repeated calls, and all-failure defaults.

Confidence Score: 5/5

The PR appears safe to merge because the corrected accounting matches the documented failure allowance and is covered across both affected modules.

No actionable regressions remain; each invocation receives an independent budget, actual failures consume it, and the next failure beyond the configured allowance is re-raised.

Important Files Changed

Filename	Overview
dspy/predict/best_of_n.py	Replaces rollout-index-based failure handling and mutable instance accounting with a local per-call failure budget.
dspy/predict/refine.py	Applies the same per-call failure-budget correction to the refinement loop.
tests/predict/test_best_of_n.py	Covers late failures, budget exhaustion, repeated invocations, and default all-failure behavior for BestOfN.
tests/predict/test_refine.py	Adds equivalent regression coverage for Refine, including feedback-compatible dummy responses.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Begin forward call] --> B[Copy configured fail_count]
    B --> C[Run next rollout]
    C --> D{Rollout pipeline succeeds?}
    D -- Yes --> E[Evaluate and retain best prediction]
    D -- No --> F[Decrement local failure budget]
    F --> G{Budget below zero?}
    G -- Yes --> H[Raise exception]
    G -- No --> I{More rollouts?}
    E --> I
    I -- Yes --> C
    I -- No --> J[Return best prediction]

Loading

_{Reviews (1): Last reviewed commit: "fix(dspy): correct Refine/BestOfN fail_c..." | Re-trigger Greptile}