Skip to content

[ENH] Fix model caching at refit and checkpointing in v2 - #2350

Open
Faakhir30 wants to merge 18 commits into
sktime:mainfrom
Faakhir30:fix_model_caching
Open

[ENH] Fix model caching at refit and checkpointing in v2#2350
Faakhir30 wants to merge 18 commits into
sktime:mainfrom
Faakhir30:fix_model_caching

Conversation

@Faakhir30

Copy link
Copy Markdown
Member

Reference Issues/PRs

Fixes #2338

What does this implement/fix? Explain your changes.

  • disable model caching completly at fit
  • fix checkpoint loading to ensure that checkpointed metadata is same as passed metadata
  • added tests for changes

Other Alternative

  • dont raise an error, instead warn and recreate the model with new metadata

Did you add any tests for the change?

Yes

PR checklist

  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
  • Added/modified tests
  • Used pre-commit hooks when committing to ensure that code is compliant with hooks. Install hooks with pre-commit install.
    To run hooks independent of commit, execute pre-commit run --all-files

@Faakhir30 Faakhir30 changed the title [ENH] Disable model caching at refit and checkpointing in v2 [ENH] Fix model caching at refit and checkpointing in v2 Jul 21, 2026
@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.63585% with 37 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@e7c7965). Learn more about missing BASE report.

Files with missing lines Patch % Lines
pytorch_forecasting/base/_base_pkg.py 87.76% 17 Missing ⚠️
pytorch_forecasting/callbacks/artifact_registry.py 84.12% 10 Missing ⚠️
...forecasting/data/data_module/_tslib_data_module.py 69.56% 7 Missing ⚠️
...g/data/data_module/_encoder_decoder_data_module.py 90.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2350   +/-   ##
=======================================
  Coverage        ?   87.31%           
=======================================
  Files           ?      172           
  Lines           ?    10345           
  Branches        ?        0           
=======================================
  Hits            ?     9033           
  Misses          ?     1312           
  Partials        ?        0           
Flag Coverage Δ
cpu 87.31% <89.63%> (?)
pytest 87.31% <89.63%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@phoeenniixx phoeenniixx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will it make sense to stack this on #2323 ?

@Faakhir30

Copy link
Copy Markdown
Member Author

Will it make sense to stack this on #2323 ?

Only thing overlapping is just different place to read checkpoint metadata, concept of rebuilding model/validating metadata for checkpoint will stay the same. afaik

While solving merge conflicts: we will have to read from datamodule_metadata path instead of Path(self.ckpt_path).parent / "metadata.pkl"

@Faakhir30
Faakhir30 requested a review from phoeenniixx July 24, 2026 13:25
@phoeenniixx

Copy link
Copy Markdown
Member

Will it make sense to stack this on #2323 ?

Only thing overlapping is just different place to read checkpoint metadata, concept of rebuilding model/validating metadata for checkpoint will stay the same. afaik

While solving merge conflicts: we will have to read from datamodule_metadata path instead of Path(self.ckpt_path).parent / "metadata.pkl"

I see, but still I think we should stack these PRs, this will help me review as well - based on the new save and load interface and not on the last one :)

@phoeenniixx phoeenniixx added enhancement New feature or request module:models ptf-v2 Related to `pytorch-forecasting` v2 labels Jul 26, 2026
Signed-off-by: Faakhir30 <zahidfaakhir@gmail.com>
Signed-off-by: Faakhir30 <zahidfaakhir@gmail.com>
@Faakhir30

Copy link
Copy Markdown
Member Author
  • stacked
  • reimplemented
  • moved self.metadata = metadata at BaseModel. Some models were not doing this, and reading directly from metadata passed in param.
  • Failing test Test / Run pytest (macos-latest, 3.14) (pull_request) is internet connection issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request module:models ptf-v2 Related to `pytorch-forecasting` v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ENH] Enable model caching irrespective of data being passed at refit in v2

3 participants