optimizer closure, possible error in the docs: for discriminator in GAN example

[amansouri3476]

Here in this page, in the following example

# Alternating schedule for optimizer steps (e.g. GANs)
def optimizer_step(
    self,
    epoch,
    batch_idx,
    optimizer,
    optimizer_idx,
    optimizer_closure,
    on_tpu=False,
    using_native_amp=False,
    using_lbfgs=False,
):
    # update generator every step
    if optimizer_idx == 0:
        optimizer.step(closure=optimizer_closure)

    # update discriminator every 2 steps
    if optimizer_idx == 1:
        if (batch_idx + 1) % 2 == 0:
            # the closure (which includes the `training_step`) will be executed by `optimizer.step`
            optimizer.step(closure=optimizer_closure)
        else:
            # call the closure by itself to run `training_step` + `backward` without an optimizer step
            optimizer_closure()

    # ...
    # add as many optimizers as you want

what is the reason for calling optimizer_closure() in the else? i.e. if we want to update the discriminator every 2 batches, shouldn’t that else just do nothing?
Yet in the example in the doc, we're going over training step and computing gradients (backward) although we're not gonna use them with optimizer.step()
That's my confusion. Why waste time and compute over a training_step and backward that are not gonna be used. (Because the next call to optimizer_closure would zero the gradients, so there doesn’t seem to be any point in computing them in the first place.) Or are they used and I've misunderstood?

The interesting thing is that if you omit that optimizer_closure in the else, it’ll raise an error asking something like, didn’t you forget to call optimizer_closure? And I’m like I don’t wanna call it! But it seems it’s kinda forced. Could you please help figure out what's going on there?

Thanks!

[awaelchli]

Yes you are right it is not required to call the closure if you just want to skip the whole update for that optimizer_idx. However, the example code is implementing in addition also gradient accumulation (2 batches), which we can see from this part:

if (batch_idx + 1) % 2 == 0:
    # this condition is satisfied every other batch_idx, here we do the optimizer.step
else:
    # while accumulating, don't do optimizer.step(), only accumulate gradients

I agree, it can be confusing, but as best as I can tell, the example is correct. If you see the need, we can try to make the comments a bit better.

[amansouri3476]

Thanks @awaelchli for your reply. Indeed making the comments better would help. But there's another issue as well. When I don't want to accumulate gradients, i.e. I just want to put pass within the else part, I get an error (something like: you forgot to call optimizer_closure). It seems that optimizer_closure is needed anyways and is forced to have it.
Or am I wrong?

[phcarval]

Hi @amansouri3476 ! Sorry to bring back this conversation but it is still unresolved and still shows up upon searching this issue: have you found a way to pass the else without triggering the MisconfigurationException?
Thanks

[amansouri3476]

Hi @phcarval , I pulled my old code, and it seems that I did just as the example I have provided. I believe I didn't find a workaround back then, so I just put optimizer_closure() in the else clause. Sorry I don't have better news :( I hope this helps.

[phcarval]

Thanks for the answer! That is also the only thing that works for me. I decided to use manual optimization to avoid this situation altogether.