Issue1416 #1533
Issue1416 #1533
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1533 +/- ##
=========================================
Coverage ? 97.11%
=========================================
Files ? 55
Lines ? 2077
Branches ? 341
=========================================
Hits ? 2017
Misses ? 31
Partials ? 29
|
|
@ptrcklv thanks for taking this on! One of the maintainers will take a look soon! |
There was a problem hiding this comment.
@ptrcklv — thanks so much for this contribution and great documentation!!
left a few formatting comments / requests for additional docs. please re-request a review once you've made changes!
| trainer1.optimizer.state_dict()["state"][k]["exp_avg"], | ||
| trainer2.optimizer.state_dict()["state"][k]["exp_avg"], |
There was a problem hiding this comment.
why are we only checking equivalence of these fields? could we check the entire state_dict's values?
| @@ -216,6 +216,35 @@ def test_warmup(self): | |||
| trainer.fit(model, [dataloaders[0]]) | |||
| self.assertEqual(trainer.warmup_steps, 1) | |||
|
|
|||
| def test_save_load(self): | |||
| fd, checkpoint_path = tempfile.mkstemp() | |||
There was a problem hiding this comment.
can we put this in a try/except/finally or use a context with tempfile.NamedTemporaryFile() as f: to ensure proper cleanup?
There was a problem hiding this comment.
I put a NamedTemporaryFile, however, the tempfile.mkstemp() I copied from test_save_load() from test_multitask_classifier.py. Maybe you want to update it there, too?
There was a problem hiding this comment.
got it — yes, we likely need to clean up other parts of the codebase as well. :)
|
|
||
| Parameters | ||
| ---------- | ||
| trainer_path : |
There was a problem hiding this comment.
nit: no : here. see https://github.com/snorkel-team/snorkel/blob/master/snorkel/classification/multitask_classifier.py for example of doc strings
| @@ -216,6 +216,35 @@ def test_warmup(self): | |||
| trainer.fit(model, [dataloaders[0]]) | |||
| self.assertEqual(trainer.warmup_steps, 1) | |||
|
|
|||
| def test_save_load(self): | |||
There was a problem hiding this comment.
have you tested this with resuming training for a saved checkpoint?
There was a problem hiding this comment.
yes, I have included it in the test now
| trainer_path : | ||
| The path to the saved trainer config to be loaded | ||
| model : | ||
| MultitaskClassifier for which the optimizer has been set. Parameters of optimizer must fit to model parameters. This model | ||
| shall be the model which was fit by the stored Trainer. |
There was a problem hiding this comment.
nit: no : for parameters (see above)
Description of proposed changes
save() and load() method for Trainer to serialize the optimizer + trainer config (dependent on whether model has been fitted with that Trainer instance)
Related issue(s)
1416
Fixes # (issue)
1416
Test plan
test_trainer.py adapted save_load_test()
Checklist
Need help on these? Just ask!
tox -e complexand/ortox -e sparkif appropriate.