Skip to content

save/load ML models#120

Merged
SCiarella merged 15 commits into
mainfrom
ml_saveload
Jul 6, 2026
Merged

save/load ML models#120
SCiarella merged 15 commits into
mainfrom
ml_saveload

Conversation

@SCiarella

@SCiarella SCiarella commented May 19, 2026

Copy link
Copy Markdown
Collaborator

This closes #110, by adding a save/load utility for the ML modules using safetensors

Main changes:

  • add safetensors as a dependency
  • add diffwofost.ml_models.io with:
    • save_model(model, ...)
    • load_model(path, ...)
  • store enough metadata to rebuild a saved model
  • give models a default checkpoint name based on model structure
  • add tests for save/load
  • show how to use save/load in hybrid notebook
  • add new function to API (docs)

Design choices:

  • save_model(model) saves to a default temp location (ignored by git)
  • the default filename depends on model class + init kwargs (deterministically)
  • so the same model structure reuses the same checkpoint path
  • saving again after retraining overwrites that file
  • if you want multiple saved variants, you have to pass a custom filename

@SCiarella SCiarella requested a review from SarahAlidoost May 19, 2026 09:20

@SarahAlidoost SarahAlidoost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SCiarella thanks for implementing this 👍 . The io function looks great for saving and loading ML models! We want something similar for crop models and their parameters too. I noticed issue #110 didn't have much detail, so I added some explanations and examples to give a better idea of what I had in mind. The examples are just meant to explain the concept, the actual implementation can be different. We can also chat and brainstorm this offline!

@SCiarella

Copy link
Copy Markdown
Collaborator Author

Thanks @SarahAlidoost, I have totally missed that part!

In the new commit, I have added unified save_model and load_model functions to io.py that handle both ML models (via safetensors with state_dict() and constructor metadata) and physical models (Configuration + ParameterProvider, saved as a directory with config.json, parameters.safetensors, parameters.json, and any embedded CROP_NN_MODEL or CROP_COMPONENTS ML models).

I have also added some tests for the physical model io.

@SCiarella SCiarella requested a review from SarahAlidoost July 3, 2026 06:26

@SarahAlidoost SarahAlidoost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SCiarella thanks! look great! 🥇 I left a few comments to help improve readability; we can move some parts of save_model and load_model into config.py, but we don't need to add tests for those methods in test_config.py, the existing io tests should be enough.

I also noticed that io.py is currently under ml_models. Could please you move it one level up to src/diffwofost?

Other than that, this looks great and should be ready to merge.

Comment thread src/diffwofost/ml_models/io.py Outdated
Comment thread src/diffwofost/ml_models/io.py Outdated
Comment thread src/diffwofost/ml_models/io.py Outdated
Comment thread src/diffwofost/ml_models/io.py Outdated
Comment thread tests/ml_models/test_io.py Outdated
Comment thread tests/test_io.py
@SCiarella SCiarella requested a review from SarahAlidoost July 4, 2026 08:34

@SarahAlidoost SarahAlidoost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SCiarella thanks for addressing the comments. Only one of the suggestions has not been addressed yet, see my comment. Do you think we can also implement the classmethod from_dict or is there any issue with that? thanks! After that, we can merge this PR.

@sonarqubecloud

sonarqubecloud Bot commented Jul 6, 2026

Copy link
Copy Markdown

@SCiarella SCiarella merged commit 902a3c6 into main Jul 6, 2026
11 checks passed
@SCiarella SCiarella deleted the ml_saveload branch July 6, 2026 14:32
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.

[Task]: Save/load models

2 participants