add current_epoch to dumped_params

[maxjeblick]

What does this PR do?

Fixes #3260 by restoring the current_epoch after finishing the batch size finder.

[codecov]

Codecov Report

Merging #3261 into master will increase coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #3261    +/-   ##
=======================================
+ Coverage      86%     87%    +1%     
=======================================
  Files         117     117            
  Lines        9353    9618   +265     
=======================================
+ Hits         8074    8357   +283     
+ Misses       1279    1261    -18     

[rohitgr7]

global_step, total_batch_idx, or maybe more attributes need to be reset too. I believe a better solution would be to make a pre_fit method or something in Trainer and reassign them to their default values every time .fit is called by calling it in .fit itself.

[awaelchli]

@rohitgr7 There is a Tuner class coming, #3160 in which I believe the resetting problem will be tackled.

[rohitgr7]

Nice!! @awaelchli will it support user-defined callbacks for lr_find and auto_scale_batch_size?

[awaelchli]

I'm not sure, but we recently added support to persist state of callbacks, so we should be able to dump their state too and restore them easily if that's the problem? For further questions better ask directly here #3160

[maxjeblick]

The Tuner class looks really promising!

[SkafteNicki]

Just to update, the HyperTuner class is not going to happen at the moment, instead the tuner algorithms will be moved to a separate trainer.tune() method instead of being called by fit (it has already started #3293). So we need to fix this problem when the refactor is done.

[mergify]

This pull request is now in conflict... :(

[Borda]

@maxjeblick mind add a test to check that it is fixed...

[SkafteNicki]

LGTM

[awaelchli]

there are probably lots of other trainer attributes that need to be dumped, for example global_step?

[SkafteNicki]

@awaelchli you are probably right. I think the tuner algorithms needs an refactoring after v1.0.

[rohitgr7]

Shouldn't tuner be refactored in such a way that it won't call .fit again? Then we won't need to dump and restore anything. What it should do is just suggest and user should change the values and rerun?

[SkafteNicki]

@rohitgr7 we use fit internally in these algorithms so we don´t have to redefine the training loop necessary to run these algorithms. However, IMO moving forward we should refactor it in such a way that tuner should just create a new trainer that can be used during tuning and then destroyed afterwards.

[rohitgr7]

@SkafteNicki I am not suggesting to rewrite anything. The reason we need to dump and restore is because these attributes are not re-initialized and current workflow is something like:

trainer.tuner.lr_find()
# suggest
trainer.fit()

What I am suggesting is:

trainer.tuner.lr_find()
# suggest
# and tell user to reinitalize trainer, LM, and LDM(if any) with the updated hparams from the suggestions.

IMO moving forward we should refactor it in such a way that tuner should just create a new trainer that can be used during tuning and then destroyed afterwards.

this is good alternative but this will work for only Trainer and not for LM and LDM since there are chances that some attributes might be changed during .fit().