Skip to content

fix(dspy): reconcile max_tokens override with aliased max_completion_tokens for reasoning models - #79

Open
detail-app[bot] wants to merge 2 commits into
mainfrom
detail/bug-fix/fix-dspy-reconcile-max-tokens-override-with-aliase-a1fcfa
Open

fix(dspy): reconcile max_tokens override with aliased max_completion_tokens for reasoning models#79
detail-app[bot] wants to merge 2 commits into
mainfrom
detail/bug-fix/fix-dspy-reconcile-max-tokens-override-with-aliase-a1fcfa

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 6, 2026

Copy link
Copy Markdown

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

reasoning_lm.copy(max_tokens=N) on an OpenAI reasoning-model dspy.LM corrupted kwargs: it inserted a spurious max_tokens=N key next to the stale, init-renamed max_completion_tokens key instead of updating it. The dual-key state then flowed into the litellm request on both model_type="chat" and model_type="responses" paths (contradictory token-budget params), and dump_state() silently overwrote the user's N with the stale value on save/reload.

Root cause: LM.__init__/_get_initial_kwargs aliases the user-facing max_tokens to the provider key max_completion_tokens for reasoning models, but the generic BaseLM.copy() (which bypasses __init__ and merges overrides directly into kwargs) never knew about the alias. The compounding dump_state() site was also asymmetric with load_state() — it unconditionally overwrote max_tokens, while load_state() only sets it when absent.

Fix (dspy/clients/lm.py, two small changes):

  • Override LM.copy() to reconcile max_tokensmax_completion_tokens for reasoning models before delegating to BaseLM.copy(), so the copy matches what __init__ would produce and the override survives dump_state().
  • Harden LM.dump_state() to only set max_tokens when absent (and always drop max_completion_tokens), mirroring the defensive load_state() — defense in depth against any other code path that might create a dual-key state.

Closes Unknown issue

✅ Contributor Checklist

  • Pre-Commit checks are passing (locally and remotely) — ruff check passes on both changed files; pre-commit run --files dspy/clients/lm.py tests/clients/test_lm.py passes
  • Title of your PR / MR corresponds to the required format — fix(dspy): reconcile max_tokens override with aliased max_completion_tokens for reasoning models
  • Commit message follows required format {label}(dspy): {message}

⚠️ Warnings

AI-generated contribution disclosure (per CONTRIBUTING.md's "AI-Generated Contributions" policy): This change was authored with the opencode CLI agent (powered by the Detail model). Prompts: the bug report itself (which included the failing test, root-cause analysis, and a recommended fix sketch) was provided as the task; I verified the bug independently by reproducing the dual-key kwargs state and the dump_state() data loss, confirmed the existing copy() test suite did not exercise the alias interaction, and confirmed via git blame/git log that the alias and the generic merge logic became jointly reachable at commit 80cdcbef. The fix is deliberately minimal and mirrors DSPy's existing reasoning-model aliasing convention; no new validation surface is added.

Testing: Unit tests, lint, and build all pass. Seven focused regression tests were added to tests/clients/test_lm.py:

  • Primary regression: copy(max_tokens=N) produces a single max_completion_tokens key (not a dual-key state) and dump_state()["max_tokens"] preserves the raised budget.
  • Chat path: the litellm request dict forwarded by the copied LM contains only max_completion_tokens (no spurious max_tokens), patched via dspy.clients.lm.litellm_completion.
  • Responses path: the litellm call contains only max_output_tokens (no leaked max_completion_tokens/max_tokens), patched via litellm.responses.
  • Save/reload roundtrip: dump_state → load_state → dump_state preserves the budget at every hop.
  • copy(max_tokens=None) removes the aliased key (the standard "None means remove" copy() semantics).
  • Non-reasoning models keep using max_tokens directly (the reconciliation is correctly gated on _is_openai_reasoning_model).
  • dump_state() defense-in-depth: a simulated pre-existing dual-key state keeps the existing max_tokens and drops the stale alias.

Full tests/clients/test_lm.py (94 tests) passes, as does the broader tests/clients/ suite and the DSPy-internal lm.copy() callers (tests/predict/test_best_of_n.py, tests/predict/test_refine.py, tests/teleprompt/). A sensitivity check confirmed the new tests fail without the fix (e.g. the primary test fails with unexpected token keys: ['max_completion_tokens', 'max_tokens']) and pass with it. uv build succeeds and the built wheel imports cleanly.

Not verified end-to-end: The live tests/clients/test_lm_direct_live.py probes (which make real OpenAI API calls to confirm the provider does not reject the single-budget request) could not be run — they require a valid OPENAI_API_KEY, which is unavailable in this environment. The mocked chat/responses request-path tests cover the same LM.forward → litellm code paths offline; only live-provider acceptance of the reconciled single-key request is unconfirmed.

Known scope limitation (not addressed here): BaseLM.copy() bypasses LM._get_initial_kwargs(), so it does not re-run the temperature == 1.0 / max_tokens >= 16000 validation that the constructor enforces for reasoning models. This fix reconciles the alias but does not add validation to copy(); that is a separate, pre-existing gap.


Automatic Fixes PRs can be configured here.

@greptile-apps

greptile-apps Bot commented Sep 6, 2026

Copy link
Copy Markdown

Greptile Summary

This revision makes LM.copy() choose the token-budget alias according to the copied model rather than the source model.

  • Uses the target model when deciding whether to translate max_tokens to max_completion_tokens.
  • Retains the serialization safeguards and focused regression coverage added earlier.
  • The existing cross-model finding was manually resolved, although copied kwargs can still retain the source model’s stale token key.

Confidence Score: 5/5

The PR has no reportable outstanding findings under the review-thread rules and appears safe to merge.

The previous cross-model token-alias finding was manually resolved without explanation, so it does not lower merge confidence and cannot be reposted as a new comment.

Important Files Changed

Filename Overview
dspy/clients/lm.py Uses the target model for alias selection, but generic kwargs merging can retain the source model’s opposite token-budget key during cross-model copies.
tests/clients/test_lm.py Adds comprehensive same-model alias and serialization tests, but does not cover copies across the reasoning-model boundary.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A["LM.copy(model, max_tokens)"] --> B{"Target is reasoning model?"}
  B -->|Yes| C["Translate override to max_completion_tokens"]
  B -->|No| D["Keep override as max_tokens"]
  C --> E["BaseLM.copy merges source kwargs"]
  D --> E
  E --> F["Copied LM"]
Loading

Reviews (2): Last reviewed commit: "fix(lm): use target model for token alia..." | Re-trigger Greptile

Comment thread dspy/clients/lm.py Outdated
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.

1 participant