-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add ddp_find_unused_parameters_true
alias in Fabric's DDPStrategy
#20125
Add ddp_find_unused_parameters_true
alias in Fabric's DDPStrategy
#20125
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20125 +/- ##
=======================================
Coverage 89% 89%
=======================================
Files 267 267
Lines 23076 23077 +1
=======================================
+ Hits 20577 20578 +1
Misses 2499 2499 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@01AbhiSingh That's great!
Now you need to update the tests, you can see at the bottom of the PR what's failing.
These two tests need to be updated:
FAILED test_cli.py::test_run_get_supported_strategies
FAILED strategies/test_registry.py::test_available_strategies_in_registry
ddp_find_unused_parameters_true
alias in Fabric's DDPStrategy
Hey @01AbhiSingh are you able to bring this over the finish line? If help is needed, let me know 👍 |
Hii @awaelchli. I will update the PR asap, Sorry I got stuck with my end Semester exams. Will get back to you asap. |
for more information, see https://pre-commit.ci
Hii @awaelchli . I tried to understand the remaining two tests that are not passing by looking at the details, but was not able to understand the issue, why is it failing. It would be very helpful if you could help. I was able to clear the tests which you mentioned here below.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good, thanks. Well done!
What does this PR do?
Fixes #20037
Before submitting
PR review
Following changes were made in the
src/lightning/fabric/strategies/ddp.py
:Tagging @awaelchli for visibility.
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
📚 Documentation preview 📚: https://pytorch-lightning--20125.org.readthedocs.build/en/20125/