[Tune] use epoch for ptl checkpoint dir name

[amogkam]

Why are these changes needed?

Related issue number

If using epoch level callbacks (on_epoch_start or on_epoch_end), then use trainer.current_epoch as the checkpoint directory name instead of trainer.global_step.

Closes #13971

Checks

• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[richardliaw]

will it be possible that you can have global_step and current_epoch override each other in the same training loop?

[amogkam]

Hmm I don’t think that’s possible as long as you’re using 1 callback since it’ll checkpoint at either the epoch frequency or batch frequency.

[amogkam]

But maybe I’m misunderstanding what you are asking

[richardliaw]

ok cool

[krfricke]

Hm, actually we do allow passing multiple on values to a single callback. So in that case it might interfere.
I'm not sure this is a common use case (mixing on-step and on-epoch checkpointing) but we might want to ensure we don't run into conflicts.

Things we could do: First, use a formatted string instead of an int to pass to tune.checkpoint_dir(step) (which is actually supported). Second, set self._epoch = True if any of the on indicate per-epoch checkpointing and always use epochs for checkpointing (potentially overwriting existing checkpoints at the end of an epoch). Third, disallow multiple on values for checkpoints

[krfricke]

Fourth, like thirds but throw a warning instead. I'd be fine with that.

[amogkam]

Right, passing in multiple values to on would be pretty uncommon but is still possible. I'll change it to option 1- the default PTL callbacks do the same thing.

[amogkam]

I made the change @krfricke- please take another look.

[richardliaw]

thanks!

[krfricke]

Fine with me! Thanks!

[amogkam]

@richardliaw can you merge this please