Set precision=16 when use_amp is passed as True #1145

rmrao · 2020-03-14T01:00:32Z

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 to update the docs?
Did you write any new necessary tests?
If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

Borda

rather define deprecated API

CHANGELOG.md

pytorch_lightning/trainer/trainer.py

rmrao · 2020-03-14T20:32:19Z

Made the requested changes as far as I can tell. If I missed something, let me know!

rmrao · 2020-03-14T20:47:28Z

Hmm - it looks like trainer sets use_amp at some point and maybe this causes conflicts. I will debug and test locally.

rmrao · 2020-03-15T16:43:34Z

Ok, I have found and fixed the previous issue and run tests locally, so I believe things should work.

One concern is that it looks like the trainer uses self.use_amp in a number of places. The deprecation strategy will cause this to raise deprecation warnings though, even if the user passes in precision=16. So I see two options:

Change all checks from self.use_amp to self.precision == 16 or maybe self.precision < 32.
Have the use_amp argument be deprecated, but still have the trainer set the use_amp attribute internally, without deprecated api (basically revert back to original solution)

Do you have a preference / other suggestions?

Borda · 2020-03-15T18:24:00Z

in such case, it would make sense to have use_amp as a standard property and make deprecated jut the setter to it... as we want to reduce this kind duplicated setting via use_amp and precision

rmrao · 2020-03-15T22:09:32Z

Right now, the trainer has this line

if self.precision == 16 and self.num_tpu_cores is None:
    use_amp = True
self.init_amp(use_amp)

Then in init_amp we have

def init_amp(self, use_amp):
     self.use_amp = use_amp and APEX_AVAILABLE

That means that the trainer, by default, accesses the setter for use_amp. The easiest way I see to solve this is to simply set precision = 16 when use_amp is passed in as an argument, and avoid the setter/getter property. Alternatively, the property should read something like

@property
def use_amp(self):
    return self.precision < 32 and self.num_tpu_cores is None and APEX_AVAILABLE

@use_amp.setter
def use_amp(self, use_amp):
    warnings.warn("`use_amp` has been deprecated in favor of `precision` since v0.7.0"
                             " and will be removed in v0.9.0", DeprecationWarning)
    self.precision = 16 if use_amp else 32

But I'm not sure what to do about init_amp in that case. Is this is a part of the API that can't / shouldn't be changed?

Borda

good approach, just a bit cleaning according to your comment that we need use_amp elsewhere
pls do changes as you suggest in #1145 (comment)

pytorch_lightning/trainer/deprecated_api.py

rmrao · 2020-03-15T22:45:50Z

Can make those changes. What should I do about init_amp? Right now it looks like this:

    def init_amp(self, use_amp):
        self.use_amp = use_amp and APEX_AVAILABLE
        if self.use_amp:
            log.info('Using 16bit precision.')


        if use_amp and not APEX_AVAILABLE:  # pragma: no cover
            msg = """
            You set `use_amp=True` but do not have apex installed.
            Install apex first using this guide and rerun with use_amp=True:
            https://github.com/NVIDIA/apex#linux

            this run will NOT use 16 bit precision
            """
            raise ModuleNotFoundError(msg)

That means that the trainer access the self.use_amp setter regardless of whether the user actually passes in the use_amp parameter. I can change the method signature, but this may cause problems for users if they have overridden this method. I am not sure if overriding this method is supported, but if it is, then I assume the signature needs to be kept the same.

One option is to make the following change:

    def init_amp(self, precision):
        if isinstance(precision, bool):
              # raise deprecation warning
              self.use_amp = precision

        if self.use_amp:
            log.info('Using 16bit precision.')

codecov · 2020-03-16T20:04:38Z

Codecov Report

Merging #1145 into master will decrease coverage by 0%.
The diff coverage is 93%.

@@          Coverage Diff           @@
##           master   #1145   +/-   ##
======================================
- Coverage      92%     92%   -0%     
======================================
  Files          63      63           
  Lines        3323    3330    +7     
======================================
+ Hits         3051    3057    +6     
- Misses        272     273    +1

Borda · 2020-03-17T07:55:28Z

@rmrao could you pls update from master? we have fixed the failure in #1163

Borda · 2020-03-23T10:09:27Z

@rmrao I have applied all previous suggestions, but we still need to as test for this one

mergify · 2020-03-24T18:53:45Z

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

Borda · 2020-03-27T14:54:11Z

@rmrao how is it going, could I help?

pep8speaks · 2020-03-30T22:38:06Z

Hello @rmrao! Thanks for updating this PR.

In the file pytorch_lightning/trainer/trainer.py:

Line 24:111: E501 line too long (115 > 110 characters)

Comment last updated at 2020-04-06 07:42:59 UTC

williamFalcon · 2020-03-30T22:38:37Z

@Borda can we merge this?

Borda

LGTM 🚀 @williamFalcon ^^

Borda · 2020-03-31T11:50:23Z

it seems that there is incompatibility with master... @rmrao ^^

rmrao · 2020-04-04T22:09:05Z

Ok I just went back to the easy solution. Tests passed locally, so should be good.

Borda · 2020-04-04T22:53:19Z

@rmrao it seems that I do have permission to push to your master to edit this PR, do you have enabled edit for maintained?
https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

rmrao · 2020-04-04T23:07:56Z

I definitely have edit access for maintained enabled, just double checked.

Borda · 2020-04-04T23:37:01Z

just curious, there is also use_amp in the model... it is kind of duplication @jeremyjordan?

mergify · 2020-04-05T00:06:46Z

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

Borda · 2020-04-05T00:24:11Z

@rmrao did some more cleaning, could you please check and resolve conflicts...

rmrao · 2020-04-05T00:49:16Z

It looks good to me!

williamFalcon · 2020-04-05T15:19:38Z

@rmrao mind rebasing?

Borda · 2020-04-05T21:26:55Z

@williamFalcon rebased ^^

CHANGELOG.md

* Set precision=16 when use_amp is passed as True * Update CHANGELOG.md * add use_amp to deprecated API * Update trainer.py * Update trainer.py * move the use_amp attribute to deprecated API * move use_amp deprecation back to Trainer's __init__ * drop unsed * drop deprecated * reorder imports * typing Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: William Falcon <[email protected]> Co-authored-by: J. Borovec <[email protected]>

Borda added bug Something isn't working feature Is an improvement or enhancement labels Mar 14, 2020

Borda added this to the 0.7.2 milestone Mar 14, 2020

Borda requested changes Mar 14, 2020

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

Borda requested changes Mar 15, 2020

View reviewed changes

pytorch_lightning/trainer/deprecated_api.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/deprecated_api.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/deprecated_api.py Outdated Show resolved Hide resolved

Borda requested a review from MattPainter01 March 25, 2020 14:47

Borda assigned jeffling Mar 25, 2020

Borda added the priority: 0 High priority task label Mar 27, 2020

mergify bot requested a review from a team March 30, 2020 22:33

Borda approved these changes Mar 30, 2020

View reviewed changes

Borda added the ready PRs ready to be merged label Mar 30, 2020

Borda requested review from a team, justusschock and tullie March 31, 2020 07:40

justusschock approved these changes Mar 31, 2020

View reviewed changes

Borda removed the ready PRs ready to be merged label Mar 31, 2020

Borda assigned Borda and unassigned jeremyjordan Apr 4, 2020

Borda requested a review from jeremyjordan April 4, 2020 23:14

Borda added the ready PRs ready to be merged label Apr 5, 2020

jeremyjordan approved these changes Apr 5, 2020

View reviewed changes

Borda reviewed Apr 5, 2020

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

rmrao and others added 11 commits April 6, 2020 09:42

Set precision=16 when use_amp is passed as True

a58afc6

Update CHANGELOG.md

dbdb4ea

add use_amp to deprecated API

8cdfe24

Update trainer.py

8c4634f

Update trainer.py

73ecffe

move the use_amp attribute to deprecated API

44b0dd1

move use_amp deprecation back to Trainer's __init__

95f298f

drop unsed

8793928

drop deprecated

32aba53

reorder imports

59e003d

typing

e852019

williamFalcon merged commit 4ed3027 into Lightning-AI:master Apr 6, 2020

Borda modified the milestones: v0.7., v0.7.x Apr 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set precision=16 when use_amp is passed as True #1145

Set precision=16 when use_amp is passed as True #1145

rmrao commented Mar 14, 2020

Borda left a comment

rmrao commented Mar 14, 2020

rmrao commented Mar 14, 2020

rmrao commented Mar 15, 2020

Borda commented Mar 15, 2020

rmrao commented Mar 15, 2020

Borda left a comment •

edited

Loading

rmrao commented Mar 15, 2020

codecov bot commented Mar 16, 2020 •

edited

Loading

Borda commented Mar 17, 2020

Borda commented Mar 23, 2020

mergify bot commented Mar 24, 2020

Borda commented Mar 27, 2020

pep8speaks commented Mar 30, 2020 •

edited

Loading

williamFalcon commented Mar 30, 2020

Borda left a comment •

edited

Loading

Borda commented Mar 31, 2020

rmrao commented Apr 4, 2020

Borda commented Apr 4, 2020 •

edited

Loading

rmrao commented Apr 4, 2020

Borda commented Apr 4, 2020

mergify bot commented Apr 5, 2020

Borda commented Apr 5, 2020

rmrao commented Apr 5, 2020

williamFalcon commented Apr 5, 2020

Borda commented Apr 5, 2020

Set precision=16 when use_amp is passed as True #1145

Set precision=16 when use_amp is passed as True #1145

Conversation

rmrao commented Mar 14, 2020

Before submitting

What does this PR do?

PR review

Did you have fun?

Borda left a comment

Choose a reason for hiding this comment

rmrao commented Mar 14, 2020

rmrao commented Mar 14, 2020

rmrao commented Mar 15, 2020

Borda commented Mar 15, 2020

rmrao commented Mar 15, 2020

Borda left a comment • edited Loading

Choose a reason for hiding this comment

rmrao commented Mar 15, 2020

codecov bot commented Mar 16, 2020 • edited Loading

Codecov Report

Borda commented Mar 17, 2020

Borda commented Mar 23, 2020

mergify bot commented Mar 24, 2020

Borda commented Mar 27, 2020

pep8speaks commented Mar 30, 2020 • edited Loading

Comment last updated at 2020-04-06 07:42:59 UTC

williamFalcon commented Mar 30, 2020

Borda left a comment • edited Loading

Choose a reason for hiding this comment

Borda commented Mar 31, 2020

rmrao commented Apr 4, 2020

Borda commented Apr 4, 2020 • edited Loading

rmrao commented Apr 4, 2020

Borda commented Apr 4, 2020

mergify bot commented Apr 5, 2020

Borda commented Apr 5, 2020

rmrao commented Apr 5, 2020

williamFalcon commented Apr 5, 2020

Borda commented Apr 5, 2020

Borda left a comment •

edited

Loading

codecov bot commented Mar 16, 2020 •

edited

Loading

pep8speaks commented Mar 30, 2020 •

edited

Loading

Borda left a comment •

edited

Loading

Borda commented Apr 4, 2020 •

edited

Loading