-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve DSPy saving #1889
Improve DSPy saving #1889
Conversation
ad25362
to
a717349
Compare
dspy/primitives/module.py
Outdated
def load(self, path, use_legacy_loading=False): | ||
with open(path) as f: | ||
self.load_state(ujson.loads(f.read()), use_legacy_loading=use_legacy_loading) | ||
def save(self, path, save_field_meta=False, state_only=True, metadata=None, use_json=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking out loud: I don't like that we already have so many flags, since this is our opportunity to simplify save/load a lot, rather than to make it more complicated from a DX standpoint.
More actionable: I don't like that state_only is a "double negative". Turning it off does more work. It should probably be reversed, e.g. save_program=False
by default.
Separately, do we need use_json
? It seems to me that we use JSON iff save_program == False
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense! We allow users to pickle the state when save_program == False
and use_json = False
, which handles the problem of serializing data like datetime.date
.
What about this:
- I will remove the
metadata
arg, which is not useful now. - I will remove the
use_json
arg, and use the file suffix (.json or .pkl) to determine the format for saving state. - Replace
state_only
bysave_program
.
dca7a77
to
ae72f10
Compare
ae72f10
to
e7c59a3
Compare
4d29b9e
to
339200a
Compare
339200a
to
33a44ef
Compare
dspy/predict/predict.py
Outdated
@@ -136,17 +138,20 @@ def _load_state_legacy(self, state): | |||
|
|||
return self | |||
|
|||
def load(self, path, return_self=False): | |||
def load(self, path, use_legacy_loading=False, return_self=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this method even need to exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can delete it since we have made our new plan for backward compatibility.
We are reworking DSPy saving in order to make it more robust:
state_only=False
to save the model architecture along with the state. This is done via cloudpickle.From the user perspective, it means they have 3 journeys on saving a DSPy model.
Method 1: Save everything.
Method 2 (our old approach): Save state with Json
Method 3: Save state with cloudpickle