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

Pass the scaler as an input to NativeMixedPrecisionPlugin #10055

Merged
merged 17 commits into from
Oct 28, 2021
Merged

Pass the scaler as an input to NativeMixedPrecisionPlugin #10055

merged 17 commits into from
Oct 28, 2021

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented Oct 20, 2021 •
edited
Loading

What does this PR do?

  • Avoids optionally defining the scaler attribute.
  • Avoids creating the scaler twice.
  • Lets PyTorch infer the dtype for torch>=1.10
  • Adds "fairscale available" check.

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

UNSTABLE API: The __init__ signature has changed

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?
  • [n/a] 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

  • 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

@carmocca carmocca added this to the v1.5 milestone Oct 20, 2021
@carmocca carmocca self-assigned this Oct 20, 2021
@github-actions
Copy link
Contributor

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@carmocca carmocca changed the base branch from master to feat/new-autocast October 20, 2021 17:28
@carmocca carmocca marked this pull request as draft October 20, 2021 17:28
Base automatically changed from feat/new-autocast to master October 25, 2021 17:33
@carmocca carmocca marked this pull request as ready for review October 26, 2021 01:42
@github-actions
Copy link
Contributor

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@codecov
Copy link

codecov bot commented Oct 26, 2021 •
edited
Loading

Codecov Report

Merging #10055 (6d3f60e) into master (47e7a28) will decrease coverage by 4%.
The diff coverage is 75%.

❗ Current head 6d3f60e differs from pull request most recent head 153f7f4. Consider uploading reports for the commit 153f7f4 to get more accurate results

@@           Coverage Diff            @@
##           master   #10055    +/-   ##
========================================
- Coverage      93%      89%    -4%     
========================================
  Files         182      181     -1     
  Lines       16149    16136    -13     
========================================
- Hits        14939    14313   -626     
- Misses       1210     1823   +613

tchaton
tchaton approved these changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM !

pytorch_lightning/plugins/precision/native_amp.py Show resolved Hide resolved
pytorch_lightning/plugins/precision/native_amp.py Show resolved Hide resolved
pytorch_lightning/plugins/precision/native_amp.py Show resolved Hide resolved
pytorch_lightning/plugins/precision/native_amp.py Show resolved Hide resolved
pytorch_lightning/plugins/precision/native_amp.py Show resolved Hide resolved
@awaelchli awaelchli requested a review from ananthsub October 26, 2021 10:31
@carmocca @tchaton
Update pytorch_lightning/plugins/precision/native_amp.py
153f7f4 
Co-authored-by: thomas chaton <thomas@grid.ai>
@carmocca carmocca enabled auto-merge (squash) October 26, 2021 12:41
@awaelchli
Copy link
Contributor

awaelchli commented Oct 26, 2021 •
edited
Loading

Will you add the addition of the scaler argument in the changelog? Even if the api is considered unstable, I think it should still be mentioned.

@mergify mergify bot added ready PRs ready to be merged and removed has conflicts labels Oct 26, 2021
@github-actions
Copy link
Contributor

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

2 similar comments
@github-actions
Copy link
Contributor

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions
Copy link
Contributor

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Borda
Borda approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@Borda Borda 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 mergify bot removed the has conflicts label Oct 27, 2021
@carmocca carmocca merged commit 5262b63 into master Oct 28, 2021
@carmocca carmocca deleted the refactor/scaler-as-input branch October 28, 2021 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justusschock justusschock justusschock approved these changes

@tchaton tchaton tchaton approved these changes

@Borda Borda Borda approved these changes

@awaelchli awaelchli awaelchli approved these changes

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

@rohitgr7 rohitgr7 Awaiting requested review from rohitgr7

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@edenlightning edenlightning Awaiting requested review from edenlightning

@ananthsub ananthsub Awaiting requested review from ananthsub

Assignees

@carmocca carmocca

Labels
ready PRs ready to be merged refactor
Projects
None yet
Milestone
pl:1.5
Development

Successfully merging this pull request may close these issues.
5 participants
@carmocca @awaelchli @Borda @tchaton @justusschock