Skip to content

fix(dspy): preserve JSON checkpoint on serialization failure - #10618

Open
zouyonghe wants to merge 1 commit into
stanfordnlp:mainfrom
zouyonghe:fix/json-checkpoint-serialization
Open

zouyonghe wants to merge 1 commit into
stanfordnlp:mainfrom
zouyonghe:fix/json-checkpoint-serialization

Conversation

@zouyonghe

Copy link
Copy Markdown

📝 Changes Description

Prevent JSON serialization errors from destroying an existing DSPy checkpoint.

The JSON branch of BaseModule.save() opens the destination with "wb" before calling orjson.dumps. If the new state contains a non-JSON-serializable value, serialization raises as intended, but opening the file has already erased the last valid checkpoint. Saving a normal Predict("question -> answer"), then adding an unserializable value to traces and saving again reproduces this without an LM: a 483-byte checkpoint becomes empty.

Serialize inside the existing error handler before opening the file, then write the resulting bytes. This preserves the existing error message and valid output format, avoids creating an empty file on an initial failed save, and leaves an existing checkpoint untouched on serialization failure. Pickle and whole-program saving are unchanged.

Verification

  • New parametrized test fails on unchanged code for both existing and absent destinations.
  • After the fix, pytest tests/primitives/test_module.py tests/predict/test_predict.py -q: 99 passed, with two existing warnings.
  • The test verifies exact previous bytes and successfully reloads the preserved checkpoint.
  • Ruff lint, relevant repository pre-commit checks, and git diff --check passed.
  • Five-axis self-review completed before publication; no blocking finding in this scope.

✅ Contributor Checklist

  • Relevant pre-commit checks pass locally; remote checks pending.
  • Conventional PR title and commit message.
  • Regression confirmed red→green and full diff reviewed before submission.

⚠️ Warnings

AI assistance used for investigation, code, tests, and self-review. This is not atomic file replacement: I/O errors or a process crash after opening the destination can still affect it. Full repository tests and cross-platform behavior were not verified; no real model/provider was called. Whole-file formatting was not changed because existing formatting differences are outside this patch.

@greptile-apps

greptile-apps Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium impact] Safe to merge within the stated scope: JSON serialization failures preserve existing checkpoints and do not create empty files at new destinations.

T-Rex evidence

Evidence from the check

  • Runs the requested tests and both comparison probes, capturing commands, working directories, exit codes, and observed output for reproducibility.

Evidence from the check

  • Exercises actual base and PR save implementations with filesystem assertions and reloads, establishing the behavior change without modifying tracked code.

Command output from the check

  • Ran the added parameterized regression in the prepared environment; both destination cases passed.

Command output from the check

  • Ran both requested adjacent test files; all 99 tests passed with one unrelated deprecation warning.

Command output from the check

  • Executed the base save implementation on unsupported traces; it created an absent file and truncated an existing checkpoint.

Command output from the check

  • Executed the PR save implementation on the same cases; absence and exact checkpoint bytes were preserved, and reload succeeded.

View artifacts

Summary

This PR serializes JSON before opening the checkpoint file, so serialization errors no longer erase an existing checkpoint.

  • JSON saves keep their checkpoint when state cannot be encoded.

No blocking issues were found. This change is safe to merge within its stated scope.

Reviews (1) · Last reviewed commit: "fix(dspy): preserve JSON checkpoint on s..." · Reviewed by Greptile

This branch has not been deployed

No deployments
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