Add configure_gradient_clipping hook in LightningModule

[rohitgr7]

What does this PR do?

Fixes #6346

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

• Was this discussed/approved via a GitHub issue? (not for typos and docs)
• 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?
• Did you make sure to update the documentation with your changes? (if necessary)
• Did you write any new necessary tests? (not for typos and docs)
• Did you verify new and existing tests pass locally with your changes?
• Did you list all the breaking changes introduced by this pull request?
• Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

• Is this pull request ready for review? (if not, please submit in draft mode)
• Check that all items from Before submitting are resolved
• Make sure the title is self-explanatory and the description concisely explains the PR
• Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

[rohitgr7]

@carmocca should we deprecate gradient_clip_val and gradient_clip_hook arguments from Trainer? Also initial thoughts on this please :)

[ananthsub]

@rohitgr7 imo we should discuss this more on the issue. I think there are some design aspects that have to be worked out first

[awaelchli]

I don't understand this PR. It seems to be incomplete.
Today I am doing this:

Trainer(gradient_clip_val=15.0)

or

python train.py --trainer.gradient_clip_val 15.0

This PR aims at deprecating this option. How do I achieve the above now? There seems to be a hook attempting to replace this functionality, but this is missing the point why the Trainer argument exists in the first place. 99.9% of the time the user does not need to implement gradient clipping because Lightning can do it. But by removing this now, you force the user to 100% of the time have to implement a hook? IMO you are removing a perfectly fine feature and I vote against going in this direction. If configurability is desired, it should be entirely optional.

[rohitgr7]

so for now I have implemented option 2 from here: #6346 (comment)

feel free to put your suggestions/recommendations here or on the issue :)

[codecov]

Codecov Report

Merging #9584 (84003ec) into master (b530b7a) will increase coverage by 0%.
The diff coverage is 93%.

@@          Coverage Diff           @@
##           master   #9584   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         178     178           
  Lines       15652   15717   +65     
======================================
+ Hits        14508   14572   +64     
- Misses       1144    1145    +1     

[tchaton]

LGTM ! Nice work :)

[awaelchli]

while this addition is probably needed and desired from a practical point of view, it also shows where Lightning's automatic optimization breaks down imo, because the desire of configuration here means ignoring settings that were provided through the trainer, which then in turn also challenges reproducibility.

[rohitgr7]

while this addition is probably needed and desired from a practical point of view, it also shows where Lightning's automatic optimization breaks down imo, because the desire of configuration here means ignoring settings that were provided through the trainer, which then in turn also challenges reproducibility.

settings provided in the trainer will still work and one doesn't need to change their code at all since those trainer args are passed here by default. This is completely optional for those who want to configure it as per their requirement. Doesn't break automatic optimization.

[awaelchli]

Yes it does! Let me make it explicit with an example:

def configure_gradient_clipping(self,optimizer, optimizer_idx, gradient_clip_val, gradient_clip_algorithm)
    if optimizer_idx == 0:
        self.clip_gradients(optimizer, optimizer_idx, 10, "norm")
    else:
        self.clip_gradients(optimizer, optimizer_idx, 20, "norm")

Now:

trainer = Trainer(gradient_clip_val=30)  # has no effect!!!

Same issue when configuration is done externally:

python train.py --gradient_clip_val=30 

This is where Lightning reaches the limit and essentially breaks. This is an issue we will face more in the future.

[rohitgr7]

okay.. so for this what we can do is make these two trainer arguments None by default. Now inside clip_gradients we can check:

1. if they are not None and different value is passed, then we can raise a warning/misconfigexception.
2. if they are not None and same value is passed, then it's fine.
3. if they are None and different value is passed, use the new value
4. if they are None, and no value is passed, then use the current default values that we have in Trainer (0.0 and 'norm')

wdyt @awaelchli ?