Fix/load weights from ckpt with encoder or likelihood #1744

madtoinou · 2023-05-04T10:56:36Z

Summary

Add the new argument load_encoders (default: True) to load_weights() and load_weights_from_checkpoints() so that the model can run inference directly after loading the weights with the encoders.
- If load_encoders=True, the model loading the weights must be instantiated without encoder or with the same encoders, otherwise an exception is raised.
- If load_encoders=False and the weights were trained with encoders, the model loading the weights must already be instantiated with encoders (identical or different than the ones used during the first training, must be careful about the dimensions) otherwise an exception is raised.
Add __eq__ operator to the LikelihoodModel (checking their numerical parameters only) and DataTransformer to prevent weights loading failure occurring during the hyper-parameters sanity check.

Other Information

I don't know if it makes sense but we could eventually exclude the likelihood attribute from the hyper-parameter check when loading the weights in case some user want to fine-tune/retrain the model with a different likelihood.

…nity checks

…ssociated unittests

fix: updated the PR reference in the changelog

…w model ckpt

…omponents dimensions for both past and future covariates), extended the unitests

madtoinou · 2023-05-08T15:33:37Z

Extended the PR:

the new encoders are instantiated in load_weights_from_checkpoint() to allow for direct inference if they don't need to be fitted (no transformer for example).
some sanity check about the number of components generated by the encoders to generate an informative exception if the new encoders don't match the checkpoint ones (in case user want to use different encoders during fine-tuning).

dennisbader

Thanks @madtoinou, those are nice fixes 🚀

I left a couple of suggestions, mainly regarding the __eq__ methods (simplification, and clarification).

darts/models/forecasting/torch_forecasting_model.py

darts/utils/likelihood_models.py

darts/tests/models/forecasting/test_torch_forecasting_model.py

darts/dataprocessing/transformers/base_data_transformer.py

darts/tests/models/forecasting/test_torch_forecasting_model.py

codecov-commenter · 2023-05-15T07:44:50Z

Codecov Report

Patch coverage: 91.54% and project coverage change: -0.07 ⚠️

Comparison is base (31da6d3) 94.17% compared to head (14c38ed) 94.10%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1744      +/-   ##
==========================================
- Coverage   94.17%   94.10%   -0.07%     
==========================================
  Files         125      125              
  Lines       11513    11567      +54     
==========================================
+ Hits        10842    10885      +43     
- Misses        671      682      +11

Impacted Files	Coverage Δ
darts/dataprocessing/encoders/encoder_base.py	`95.65% <75.00%> (-0.37%)`	⬇️
darts/dataprocessing/encoders/encoders.py	`97.87% <82.35%> (-1.00%)`	⬇️
...arts/models/forecasting/torch_forecasting_model.py	`90.46% <95.23%> (+0.09%)`	⬆️
darts/utils/likelihood_models.py	`97.85% <100.00%> (+1.81%)`	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

…_encoders returns them instead of changing model attributes

…wing reviewer comments

…n Likelihood

…he same encoders but different fittable transformer

dennisbader

Very nice, thanks :) 🚀

Soon ready to be merged!
Only had some minor suggestions, and that it might be better to avoid the __eq__() and rather do a simple check when comparing add_encoders for the same transformer class.

darts/dataprocessing/encoders/encoders.py

darts/tests/models/forecasting/test_torch_forecasting_model.py

dennisbader · 2023-05-19T12:25:02Z

darts/tests/models/forecasting/test_torch_forecasting_model.py

+            )
+            model_same_likelihood.load_weights(model_path_manual, map_location="cpu")
+            model_same_likelihood.to_cpu()
+            model_same_likelihood.predict(n=4, series=self.series)


We can check that load_weights_from_checkpoint and load_weights have equal predictions.

The Likelihood object do not have the same random_state (also mentioned in #1779), making the predictions different. Should I implement a method to set it before updating this test?

see my suggestion below

darts/tests/models/forecasting/test_torch_forecasting_model.py

darts/dataprocessing/transformers/fittable_data_transformer.py

darts/dataprocessing/transformers/base_data_transformer.py

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

…a typo

dennisbader

Thanks a lot, it's very nice 🚀
A last iteration with some minor suggestions and then we're good to go 👍

darts/models/forecasting/torch_forecasting_model.py

darts/tests/models/forecasting/test_torch_forecasting_model.py

dennisbader · 2023-05-23T12:32:00Z

darts/tests/models/forecasting/test_torch_forecasting_model.py

+            )
+            model_same_likelihood.load_weights(model_path_manual, map_location="cpu")
+            model_same_likelihood.to_cpu()
+            model_same_likelihood.predict(n=4, series=self.series)


see my suggestion below

darts/tests/models/forecasting/test_torch_forecasting_model.py

…ed test for likelihood

dennisbader

Great job @madtoinou 💯 Thanks a lot 🚀

* fix: possible to load encoders from the checkpoint, including some sanity checks * feat: adding corresponding unittests * feat: adding eq operator to likelihood models and data_transformer, associated unittests * fix: removed typo, updated changelog * fix: remove typo in call to super().__eq__ * Update CHANGELOG.md fix: updated the PR reference in the changelog * fix: load encoders before weights so that they are included in the new model ckpt * feat: adding multi steps pipeline unittest * new encoders are instantiated if compatible with the old ones (same components dimensions for both past and future covariates), extended the unitests * fix: adding encoding_n_component property to SequentialEncoders, load_encoders returns them instead of changing model attributes * fix: simplifying equality operator between likelihood objecting following reviewer comments * doc: adding small comment about the ignore_attrs_equality attribute in Likelihood * fix: improve equality operator between data transformers * fix: equality ignore nn.Module * test: adding test for loading weights with likelihood * test: adding test when not loading encoders in a model with contain the same encoders but different fittable transformer * test: improving test for save/load with likelihood * Apply suggestions from code review Co-authored-by: Dennis Bader <dennis.bader@gmx.ch> * fix: addressing reviewer comments * fix: changed order of exception to make them more informative, fixed a typo * fix: typo * fix: helper function to compare models encoders and transformers, added test for likelihood --------- Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

madtoinou added 5 commits May 4, 2023 09:16

fix: possible to load encoders from the checkpoint, including some sa…

26df47d

…nity checks

feat: adding corresponding unittests

98e8c18

feat: adding eq operator to likelihood models and data_transformer, a…

4de32ff

…ssociated unittests

fix: removed typo, updated changelog

4ec4af3

fix: remove typo in call to super().__eq__

85d0588

madtoinou requested review from hrzn and dennisbader as code owners May 4, 2023 10:56

madtoinou and others added 5 commits May 4, 2023 15:53

Update CHANGELOG.md

1e52d5f

fix: updated the PR reference in the changelog

fix: load encoders before weights so that they are included in the ne…

926234d

…w model ckpt

feat: adding multi steps pipeline unittest

495fda9

Merge branch 'master' into fix/load-weights-ckpt

221bdcc

new encoders are instantiated if compatible with the old ones (same c…

4141602

…omponents dimensions for both past and future covariates), extended the unitests

dennisbader requested changes May 12, 2023

View reviewed changes

Merge branch 'master' into fix/load-weights-ckpt

00e4616

madtoinou and others added 10 commits May 15, 2023 19:11

fix: adding encoding_n_component property to SequentialEncoders, load…

19b7056

…_encoders returns them instead of changing model attributes

fix: simplifying equality operator between likelihood objecting follo…

518b769

…wing reviewer comments

doc: adding small comment about the ignore_attrs_equality attribute i…

1c9bc9c

…n Likelihood

fix: improve equality operator between data transformers

99d35ee

fix: equality ignore nn.Module

cf1c99b

test: adding test for loading weights with likelihood

8b1a8b7

test: adding test when not loading encoders in a model with contain t…

e6b3a80

…he same encoders but different fittable transformer

Merge branch 'master' into fix/load-weights-ckpt

878b2a4

Merge branch 'master' into fix/load-weights-ckpt

0230bfc

test: improving test for save/load with likelihood

731c1a2

dennisbader requested changes May 19, 2023

View reviewed changes

madtoinou and others added 2 commits May 22, 2023 08:49

Apply suggestions from code review

e0359cf

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

fix: addressing reviewer comments

989d6ba

madtoinou and others added 3 commits May 22, 2023 10:22

Merge branch 'master' into fix/load-weights-ckpt

401b363

fix: changed order of exception to make them more informative, fixed …

459aa61

…a typo

fix: typo

a166134

dennisbader requested changes May 23, 2023

View reviewed changes

madtoinou and others added 3 commits May 25, 2023 13:59

fix: helper function to compare models encoders and transformers, add…

8f9f78b

…ed test for likelihood

Merge branch 'master' into fix/load-weights-ckpt

14c38ed

Merge branch 'master' into fix/load-weights-ckpt

362b2aa

dennisbader approved these changes May 26, 2023

View reviewed changes

dennisbader merged commit 52f4004 into master May 26, 2023

dennisbader deleted the fix/load-weights-ckpt branch May 26, 2023 11:59

madtoinou mentioned this pull request Jun 7, 2023

load_weights_from_checkpoint fails with likelihood model #1816

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/load weights from ckpt with encoder or likelihood #1744

Fix/load weights from ckpt with encoder or likelihood #1744

madtoinou commented May 4, 2023

madtoinou commented May 8, 2023

dennisbader left a comment

codecov-commenter commented May 15, 2023 •

edited

Loading

dennisbader left a comment

dennisbader May 19, 2023

madtoinou May 22, 2023

dennisbader May 23, 2023

dennisbader left a comment

dennisbader May 23, 2023

dennisbader left a comment

Fix/load weights from ckpt with encoder or likelihood #1744

Fix/load weights from ckpt with encoder or likelihood #1744

Conversation

madtoinou commented May 4, 2023

Summary

Other Information

madtoinou commented May 8, 2023

dennisbader left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 15, 2023 • edited Loading

Codecov Report

dennisbader left a comment

Choose a reason for hiding this comment

dennisbader May 19, 2023

Choose a reason for hiding this comment

madtoinou May 22, 2023

Choose a reason for hiding this comment

dennisbader May 23, 2023

Choose a reason for hiding this comment

dennisbader left a comment

Choose a reason for hiding this comment

dennisbader May 23, 2023

Choose a reason for hiding this comment

dennisbader left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 15, 2023 •

edited

Loading