Feat/generic save load #1070
Feat/generic save load #1070
Conversation
|
Do you think it's worth relying on joblib (vs pickle)? It seems starting with Python 3.8 the gains are not so clear: https://stackoverflow.com/questions/12615525/what-are-the-different-use-cases-of-joblib-versus-pickle Maybe we could make a few quick trials (with a few models of different sizes) to see if there's a difference in practice? If there's no clear advantage I would vote for sticking to the simplest option. |
|
Yeah this sounds like a good idea, I'll make a few trials 👍 |
I don't have a strong opinion so feel also free to leave joblib. |
|
Did a few comparisons between |
|
@brunnedu what's the status of this PR? Anything more remaining to be done? |
…eneric_save_load
…arts into feat/generic_save_load
Codecov Report
@@ Coverage Diff @@
## master #1070 +/- ##
==========================================
- Coverage 93.73% 93.64% -0.09%
==========================================
Files 80 80
Lines 8266 8282 +16
==========================================
+ Hits 7748 7756 +8
- Misses 518 526 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
brunnedu
requested review from
tomasvanpottelbergh and
dennisbader
as code owners
There was a problem hiding this comment.
Looks good to me as well :) Thanks for this, finally we have built in saving and loading for all models!
Just had a few minor comments/additions to @hrzn's comments
darts/tests/models/forecasting/test_local_forecasting_models.py
Outdated
Show resolved
Hide resolved
|
|
||
| Creates two files under ``path`` (model object) and ``path``.ckpt (checkpoint). | ||
|
|
||
| Example for saving and loading a :class:`RNNModel`: |
Addresses #936
First version of generic save and load methods for
ForecastingModelsFor now, we have decided to implement generic save and load methods for all
ForecastingModelsusing joblib, as long as there is no better alternative for a particular model subclass, as is currently the case only forTorchForecastingModels. We decided to take this approach because most libraries either suggest doing it this way (sklearn) or wrap a very similar solution themselves (statsmodels).There are obvious downsides to this solution regarding maintainability and security such as the following ones:
ForecastingModelssaved in one version of darts might not be loadable in another version of darts or they could exhibit unexpected behavior.ForecastingModel.load_model()on untrusted data as it could lead to malicious code being executed upon loading.I'm open to hearing your opinion on the current implementation and the additional functionality you believe those methods should contain.