Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

allow using apex with any PT version #2865

Merged
merged 21 commits into from
Aug 8, 2020
Merged

allow using apex with any PT version #2865

merged 21 commits into from
Aug 8, 2020

Conversation

Borda
Copy link
Member

@Borda Borda commented Aug 7, 2020 •
edited by awaelchli
Loading

What does this PR do?

add extra argument which would switch between using Apex and native AMP in pt>=1.6

Fixes #2788

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 Borda added feature Is an improvement or enhancement priority: 0 High priority task allowed_pre_1.0 design Includes a design discussion labels Aug 7, 2020
@mergify mergify bot requested a review from a team August 7, 2020 15:04
@Borda Borda marked this pull request as ready for review August 7, 2020 19:01
@codecov
Copy link

codecov bot commented Aug 7, 2020 •
edited
Loading

Codecov Report

Merging #2865 into master will increase coverage by 4%.
The diff coverage is 97%.

@@           Coverage Diff           @@
##           master   #2865    +/-   ##
=======================================
+ Coverage      86%     90%    +4%     
=======================================
  Files          79      79            
  Lines        7226    7233     +7     
=======================================
+ Hits         6214    6527   +313     
+ Misses       1012     706   -306

nateraw
nateraw approved these changes Aug 7, 2020
View reviewed changes
Copy link
Contributor

@nateraw nateraw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify
Copy link
Contributor

mergify bot commented Aug 7, 2020

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

@Borda Borda force-pushed the feature/amp-apex branch from c50e62b to 1a6119d Compare August 7, 2020 21:24
@Borda Borda added the ready PRs ready to be merged label Aug 7, 2020
@mergify
Copy link
Contributor

mergify bot commented Aug 7, 2020

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

@Borda Borda force-pushed the feature/amp-apex branch from 1a6119d to 3509013 Compare August 7, 2020 22:44
awaelchli
awaelchli suggested changes Aug 8, 2020
View reviewed changes
pytorch_lightning/accelerators/ddp_backend.py Show resolved Hide resolved
pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved
pytorch_lightning/core/memory.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/auto_mix_precision.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/auto_mix_precision.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/auto_mix_precision.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Show resolved Hide resolved
pytorch_lightning/utilities/__init__.py Show resolved Hide resolved
@awaelchli
Copy link
Contributor

@Borda I like this PR a lot!! Nice simplifications!

@mergify mergify bot requested a review from a team August 8, 2020 07:15
@pep8speaks
Copy link

pep8speaks commented Aug 8, 2020 •
edited
Loading

Hello @Borda! Thanks for updating this PR.

Line 870:120: E501 line too long (129 > 119 characters)
Line 876:1: W293 blank line contains whitespace

Comment last updated at 2020-08-08 08:32:20 UTC

Borda and others added 2 commits August 8, 2020 09:23
@Borda
Copy link
Member Author

Borda commented Aug 8, 2020 •
edited
Loading

[ ] Todo, remove all amp todoss... #2865 (comment)

@mergify mergify bot requested a review from a team August 8, 2020 08:32
@Borda Borda requested a review from awaelchli August 8, 2020 08:45
awaelchli
awaelchli approved these changes Aug 8, 2020
View reviewed changes
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, you can directly parse the input args to a type like this

self.amp_type = AMPType[amp_type.upper()]

anyway, I like this PR :)

@awaelchli
Copy link
Contributor

@Borda I linked #2788, ok?

@Borda
Copy link
Member Author

Borda commented Aug 8, 2020

btw, you can directly parse the input args to a type like this

self.amp_type = AMPType[amp_type.upper()]

anyway, I like this PR :)

Cool! that is nice, but in this case, we also need to verify if it is available...

@Borda Borda merged commit a6e7aa7 into master Aug 8, 2020
@Borda Borda deleted the feature/amp-apex branch August 8, 2020 09:07
This was referenced Aug 8, 2020
Closed
Merged
@Borda Borda modified the milestones: 1.0.0, 0.9.0 Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nateraw nateraw nateraw approved these changes

@justusschock justusschock justusschock approved these changes

@awaelchli awaelchli awaelchli approved these changes

@ethanwharris ethanwharris Awaiting requested review from ethanwharris ethanwharris is a code owner

@rohitgr7 rohitgr7 Awaiting requested review from rohitgr7

@SkafteNicki SkafteNicki Awaiting requested review from SkafteNicki

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
design Includes a design discussion feature Is an improvement or enhancement priority: 0 High priority task ready PRs ready to be merged
Projects
None yet
Milestone
pl:0.9
Development

Successfully merging this pull request may close these issues.

Enable option to use Apex when PyTorch 1.6 is installed
5 participants
@Borda @awaelchli @pep8speaks @justusschock @nateraw