[FR] Warn if scheduler.step() is called but optim.step has not been called

[ssnl]

In 1.1 we made a major BC breaking change, where the order of calling lr schedulers should be changed from

for e in range(nepochs):
  scheduler.step()
  train()

to

for e in range(nepochs):
  train()
  scheduler.step()

This silently breaks many code, and makes it impossible to write consistent code for 1.0.1 and 1.1. So I propose to add a warning in scheduler.step where it looks at the corresponding optimizer, and checks if its .step has been called.

If it has not been called, this is a sign that the user is using scheduler.step() with the old pattern. I can't think of a reasonable case where this would detect a false positive.

[ssnl]

cc @vfdev-5

[apaszke]

Why did that change??

[wwoods]

Like @apaszke, I'm wondering why this change happened? It is a rather busy-body change that does not improve user-facing pytorch code in any way, as far as I can tell. Furthermore, this complains about perfectly fine code which called step(epoch) explicitly, rather than relying on last_epoch in the _LRScheduler code. It now issues a warning incorrectly: step(0) being called prior to the optimizer step results in a warning being issued, despite implementing the correct behavior.

[vincentqb]

The change was introduced in #7889, and documented in #20203.

[ruppeshnalwaya1993]

@ssnl The warning says in pytorch 1.1.0 and later optimizer step should be before lr scheduler step. But the warning does not appear in pytorch 1.1.0. It only appears in 1.2.0 and later. Is this intended or a mistake?

1.1.0 file
https://github.com/pytorch/pytorch/blob/v1.1.0/torch/optim/lr_scheduler.py

[ssnl]

The change was in 1.1. The warning was added in 1.2.

…

On Sat, Nov 2, 2019 at 01:14 RUPPESH NALWAYA ***@***.***> wrote: @ssnl <https://github.com/SsnL> The warning says in pytorch 1.1.0 and later optimizer step should be before lr scheduler step. But the warning does not appear in pytorch 1.1.0. It only appears in 1.2.0 and later. Is this intended or a mistake? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20124?email_source=notifications&email_token=ABLJMZK4C547ZQYIW3XSRGDQRUEE5A5CNFSM4HKYGAEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC4UDQI#issuecomment-549011905>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLJMZNXR64EB5STX3EK7FDQRUEE5ANCNFSM4HKYGAEA> .

[antimora]

@ssnl, @vincentqb the implementation assumes the scheduler (fully initialized) exists prior calling optimizer.step(). The scheduler monkey patches optimizer object! I get the warning despite the scheduler.step is called after optimizer.step() because my scheduler is initialized after optimizer.step() call (lazy eval pattern). I wish your implementation did not monkey patch the optimizer object. This behavior is definitely not documented and leads to chasing the issue in wrong places.

[vfdev-5]

As it was discussed #21460 (comment) maybe it is time to remove it ...
cc @ezyang

[ssnl]

@antimora Yes I fully agree that it is bad practice. It was a decision we had to make because of the silent correctness issues. I vote for removing it. Let's see what @ezyang thinks!

[vincentqb]

Since the warning was added in 1.2 and we are now about to release 1.6, I also agree we can remove the warning to check this. I would make sure the documentation does specify the order though :) Thoughts?

[antimora]

I am in favor to remove the warning. Should I file a ticket to get it done?

(CCing @ezyang since everyone is CCing him here)

[ezyang]

Yeah let's can it. cc @soumith