set transformer to evaluation mode

[ArjunSubramonian]

Fixes #4895 .

Changes proposed in this pull request:

• If train_parameters in PretrainedTransformerEmbedder is False, the transformer's dropout and batch normalization layers are now set to evaluation mode.

[epwalsh]

Nice job! I just have a couple comments.

[epwalsh]

We already have a ### Changed section on line 13 below, you can just put this bullet point there.

[ArjunSubramonian]

Done!

[epwalsh]

This might not be necessary anymore

[ArjunSubramonian]

Got it. This Stack Overflow post says that it's still best practice to turn off gradients computation. Thoughts?

[epwalsh]

Reading that, sounds like we may want to use with torch.no_grad() in the forward pass?

I'm also curious what happens after this class is initialized as part of a submodule for a Model, when Model.train() is called. Will that revert the .eval() call within this submodule?

[epwalsh]

Might be worth having a test for that

[ArjunSubramonian]

Nice catch! I fixed this issue, albeit in a hacky way (https://stackoverflow.com/questions/61980943/how-can-i-keep-a-pytorch-submodule-in-eval-mode, https://stackoverflow.com/questions/394770/override-a-method-at-instance-level). Let me know what you think. I also added a test case to ensure that PretrainedTrransformerEmbedder's transformer model remains in eval mode even when module that instantiates it is switched to training mode.

[ArjunSubramonian]

Using with torch.no_grad() temporarily sets require_grad=False. Using it conditionally is pretty cumbersome because it would require setting up a ContextManager wrapper (https://stackoverflow.com/questions/22226708/can-a-with-statement-be-used-conditionally). I think this is why the original code explicitly sets require_grad=False for all parameters. I also don't see any other conditional uses of with torch.no_grad(). That's why I'm against it. @AkshitaB do you have an opinion on this?

[epwalsh]

One thing you could do is just rename the existing .forward() method to ._forward(), and then implement a new .foward() like:

def forward(self, ...):
    if self.train_parameters:
        return self._forward(...)
    else:
        with torch.no_grad():
            return self._foward(...)

[epwalsh]

... that said, I just realized there are other places in our code-base that would expect requires_grad to be False when this module is initialized, if we're not training it. So yea, I think we do need to keep these lines.

[epwalsh]

This makes me a bit nervous. Another option would be to add something like this to the .forward() method below (after line 182):

if self.train_parameters and self.training and self.transformer_model.training:
    self.transformer_model.eval()

[ArjunSubramonian]

I like this approach much better, thanks for the suggestion :)

[ArjunSubramonian]

One issue with this though is that inspecting .training will not alway produce the expected result.

[epwalsh]

Hmm why doesn't .eval() work here?

[ArjunSubramonian]

Because .eval() just calls .train(False)

[epwalsh]

Calling .eval() or .train(False) will recursively call the same on all submodules, and also set training = False for all submodules (https://github.com/pytorch/pytorch/blob/d490e0120f32dcbb8b23e11eebd638b96b4b0898/torch/nn/modules/module.py#L1594). Isn't that what we want?

[ArjunSubramonian]

Yes, you're right, that is what we want. I just updated the code to be more robust.

[epwalsh]

Instead of patching the .train() method on self.transformer model, why not just override the .train() method PretrainedTransformerEmbedder?

We could implement it like

@overrides
def train(self, mode: bool = True):
    self.training = mode
    for name, module in self.named_children():
        if not self.train_parameters and name == "transformer_model":
            module.eval()
        else:
            module.train(mode)
    return self

[ArjunSubramonian]

Ooh, okay, I agree that this is indeed a great solution. Thanks! Just incorporated it.

[epwalsh]

LGTM! Nice job!

[nelson-liu]

I realize I should have spoken up sooner about this, but would it be possible to change this to add another parameter that controls eval vs. non-eval mode, versus overriding the default behavior of train_parameters (asking before the next release goes out and it'd become a breaking change)? In particular, this behavior is pretty different from the setting where you use Elmo with requires_grad=False. In that setting, the Elmo weights actually aren't in eval mode.

There are also settings where you want the default behavior (i.e., non-eval mode, but frozen parameters). In particular, there's been recent work on trying to do parameter-efficient fine-tuning by, for instance, only fine-tuning the bias terms of the transformer (https://nlp.biu.ac.il/~yogo/bitfit.pdf). In this case, it's much more ergonomic to set train_parameters to False and then have a regex for [["^_text_field_embedder.token_embedder_tokens.transformer_model.*bias"], {"requires_grad": true}], versus the having to write a regex like [["^_text_field_embedder.token_embedder_tokens.transformer_model.*(?<!bias)$"], {"requires_grad": false}].

Lastly, I think that semantically train_parameters doesn't necessarily imply eval mode---they seem like distinct ways of modifying the model (e.g., how the Elmo embedder works right now).