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.

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

Fix to Parameters to MixedPrecisionPlugin are not validated and do not match doc string #17687

Merged
merged 16 commits into from
Jun 7, 2023

Conversation

akreuzer
Copy link
Contributor

What does this PR do?

Fixes #17288

Before submitting
  • Was this discussed/agreed via a GitHub issue? yes
  • 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) yes
  • Did you write any new necessary tests? (not for typos and docs) yes
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request? No breaking changes
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors) No only minor change

PR review

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

Reviewer checklist
  • 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

@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label May 23, 2023
Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

Glad that you found time!

We should also update:

src/lightning/pytorch/plugins/precision/amp.py Outdated Show resolved Hide resolved
tests/tests_pytorch/plugins/test_amp_plugins.py Outdated Show resolved Hide resolved
@carmocca carmocca added bug Something isn't working fabric lightning.fabric.Fabric precision: amp Automatic Mixed Precision trainer community This PR is from the community labels May 24, 2023
@carmocca carmocca added this to the 2.0.x milestone May 24, 2023
@carmocca carmocca removed the trainer label May 24, 2023
@carmocca carmocca self-assigned this May 24, 2023
Alexander Kreuzer added 2 commits May 24, 2023 11:56
- Improved Exception Message
- Added match to pytest exception check
- Added parameter validation to fabric
@akreuzer
Copy link
Contributor Author

akreuzer commented May 24, 2023

Address the comments, added parameter validation to fabric (docstring is already correct there), added a line to each CHANGELOG.

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #17687 (0166e1f) into master (420eb6f) will decrease coverage by 24%.
The diff coverage is 100%.

❗ Current head 0166e1f differs from pull request most recent head 017d8f0. Consider uploading reports for the commit 017d8f0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #17687     +/-   ##
=========================================
- Coverage      84%      60%    -24%     
=========================================
  Files         419      414      -5     
  Lines       31721    31583    -138     
=========================================
- Hits        26634    18810   -7824     
- Misses       5087    12773   +7686     

src/lightning/fabric/plugins/precision/amp.py Outdated Show resolved Hide resolved
src/lightning/fabric/CHANGELOG.md Show resolved Hide resolved
tests/tests_fabric/plugins/precision/test_amp.py Outdated Show resolved Hide resolved
@mergify mergify bot added the ready PRs ready to be merged label May 29, 2023
Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

Test failures look real:

FAILED strategies/test_fsdp.py::test_precision_plugin_config[16-true-expected2] - ValueError: `FSDPMixedPrecisionPlugin(precision='16-true')` must be '16-mix...
FAILED strategies/test_fsdp.py::test_precision_plugin_config[bf16-true-expected3] - ValueError: `FSDPMixedPrecisionPlugin(precision='bf16-true')` must be '16-m...

@awaelchli
Copy link
Member

@carmocca 16-true and bf16-true are not yet supported in FSDPPrecision. it needs to be refactored to support this, which is why I had opened #17657

@mergify mergify bot removed the ready PRs ready to be merged label May 31, 2023
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Jun 6, 2023
@awaelchli awaelchli enabled auto-merge (squash) June 6, 2023 22:53
@awaelchli awaelchli disabled auto-merge June 6, 2023 22:53
@awaelchli awaelchli requested a review from carmocca June 6, 2023 22:53
@carmocca carmocca enabled auto-merge (squash) June 6, 2023 23:11
@carmocca carmocca merged commit f111bd4 into Lightning-AI:master Jun 7, 2023
101 checks passed
Borda pushed a commit that referenced this pull request Jun 13, 2023
…not match doc string (#17687)

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

(cherry picked from commit f111bd4)
lantiga pushed a commit that referenced this pull request Jun 22, 2023
…not match doc string (#17687)

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

(cherry picked from commit f111bd4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working community This PR is from the community fabric lightning.fabric.Fabric pl Generic label for PyTorch Lightning package precision: amp Automatic Mixed Precision ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameters to MixedPrecisionPlugin are not validated and do not match doc string
4 participants