Allow changing the logged step value in validation_step

[mauvilsa]

What does this PR do?

This fixes the bug identified in the discussion of #4102

Fixes #4102
Fixes #4203

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

@rohitgr7 please review.

[mauvilsa]

I see that unit tests are now failing. There are asserts that check what has been logged and it now differs because epoch is now automatically logged. But I understood that having epoch is a feature. What should be done here? Change the tests?

[rohitgr7]

But I understood that having epoch is a feature. What should be done here? Change the tests?

not sure about this. let's see what other reviewers say.

[williamFalcon]

i don't think this is a bug... again, you shouldn't really log validation each step... it means nothing.

[mauvilsa]

@williamFalcon let me explain the change. This is not about logging validation each step. The hard coding of the step argument to log_metrics in logger_connector.py#L202 is unnecessary since if omitted it would get the same value anyway because of logger_connector.py#L90. However, this hard coding prevents the user to override the value of step, making inconsistent the behavior between training and validation.

The only issue is the automatic logging of epoch in logger_connector.py#L89 which makes some unit tests fail. Maybe this automatic logging should be off by default and only enabled if requested by the user?

[rohitgr7]

I believe #4132 tried to solve the problem of logging epoch on the x-axis in case of *_epoch_end, not sure why it was closed. IMO this should be improved so that by default it should set global_step for step_logs and current_epoch for epoch_logs and in case if step is provided in the metrics by the user, it should be replaced with that value. WDYT? Let's see what other has to say here.

[mauvilsa]

I agree. Even better if for epoch_logs the default is the current epoch.

[mauvilsa]

From feedback it is clear that the default should be global_step.

[rohitgr7]

nice!
let's update the test accordingly, shall we? :)

[awaelchli]

please note that the log_metrics method here is recursive.
At the bottom, it is calling itself with step (before PR changes) but now step is None. This means in the recursive call, step will be None and then it will add the epoch key. That is why you see the epoch key in the test as extra. Just wanted to point that out in case this was not clear.

[awaelchli]

I think the automatic epoch logging should stay. It is very useful to have that by default, because it tells us how many steps correspond to one epoch, and in advanced loggers it allows us to change the axis of plots to epoch. So my suggestion is to adjust the tests accordingly.

[awaelchli]

works! the epoch is back <3

[codecov]

Codecov Report

Merging #4130 into master will increase coverage by 3%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #4130    +/-   ##
=======================================
+ Coverage      90%     93%    +3%     
=======================================
  Files         103     103            
  Lines        7795    7848    +53     
=======================================
+ Hits         6990    7281   +291     
+ Misses        805     567   -238     

[mergify]

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

[mergify]

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

[rohitgr7]

@mauvilsa thanks for the fix. Also next time pls make a branch(not master) and push? would be easier to test locally :)