test: verify datastore is omitted from checkpoint hparams (#148) - #232
Conversation
|
@joeloskarsson |
|
@Jayant-kernel would you be able to use the default PR template instead please? I helps with the review process to make sure we have checked everything :) Once you've done that one of us will give your work a review. Thank you for you contribution! |
|
@leifdenby thanks |
|
@joeloskarsson @leifdenby @sadamov |
|
Hello! I'm new to the repository, and am looking for ways to contribute. I was wondering if I can be assigned this issue? Thank you! |
…rams (mllam#232) Re-applies @Jayant-kernel's PR mllam#232 onto current main. The original test targeted the pre-mllam#208 API (`GraphLAM(args=..., config=..., datastore=...)` all on the top-level model). Rewritten for the post-mllam#208 layout: build a GraphLAM step predictor + ARForecaster + ForecasterModule explicitly and assert the ignore covers both heavy non-pickle-safe objects (`datastore` and the now-additional `forecaster`). Refs mllam#148. Co-Authored-By: Jayant <Jayant-kernel@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
35cb629 to
a95956f
Compare
…rams (mllam#232) Re-applies @Jayant-kernel's PR mllam#232 onto current main. The original test targeted the pre-mllam#208 API (`GraphLAM(args=..., config=..., datastore=...)` all on the top-level model). Rewritten for the post-mllam#208 layout: build a GraphLAM step predictor + ARForecaster + ForecasterModule explicitly and assert the ignore covers both heavy non-pickle-safe objects (`datastore` and the now-additional `forecaster`). Refs mllam#148. Co-Authored-By: Jayant <Jayant-kernel@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
a95956f to
57b427f
Compare
sadamov
left a comment
There was a problem hiding this comment.
Thanks @Jayant-kernel, sorry for the long wait! Rewrote the test for the post-#208 architecture (the original used GraphLAM(args=..., config=..., datastore=...) as a top-level model, which doesn't exist anymore). Renamed to test_saved_checkpoint_excludes_datastore_and_forecaster to honour @joeloskarsson's "more descriptive name" ask, and tightened the assertion to cover the now-additional forecaster field that's also in the ignore list. Merging after CI is green.
Describe your changes
This PR adds a minimal regression test to verify that the datastore object is correctly excluded from the pickled PyTorch Lightning checkpoint hyperparameters.
The
save_hyperparameters(ignore=["datastore"])call was already previously implemented inside ARModel and successfully propagates to all model subclasses viasuper().__init__(). This test simply locks in that behavior by saving a checkpoint to disk and asserting the dictionary keys, ensuring no future refactors accidentally bypass it.trainer.save_checkpoint."datastore"is absent from the savedcheckpoint["hyper_parameters"]dictionary.Issue Link
Closes #148
Type of change
Checklist before requesting a review
Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee