Fix strategy type validation in connectors #16693
Conversation
github-actions
bot
added
fabric
for more information, see https://pre-commit.ci
… into bugfix/strategy-type-validation
awaelchli
requested review from
carmocca,
justusschock and
williamFalcon
as code owners
awaelchli
added
the
fun
⚡ Required checks status: All passing 🟢Groups summary🟢 pytorch_lightning: Tests workflow
These checks are required after the changes to 🟢 pytorch_lightning: Azure GPU
These checks are required after the changes to 🟢 pytorch_lightning: Azure HPU
These checks are required after the changes to 🟢 pytorch_lightning: Azure IPU
These checks are required after the changes to 🟢 pytorch_lightning: Docs
These checks are required after the changes to 🟢 lightning_fabric: CPU workflow
These checks are required after the changes to 🟢 lightning_fabric: Azure GPU
These checks are required after the changes to 🟢 mypy
These checks are required after the changes to 🟢 installThese checks are required after the changes to 🟢 link-check
These checks are required after the changes to Thank you for your contribution! 💜
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #16693 +/- ##
=========================================
- Coverage 81% 60% -21%
=========================================
Files 445 430 -15
Lines 32623 32417 -206
=========================================
- Hits 26467 19336 -7131
- Misses 6156 13081 +6925 |
* Add .git-blame-ignore-revs (#16709) Co-authored-by: Jirka Borovec <[email protected]> * Fix strategy type validation in connectors (#16693) * Disable strict loading in multiprocessing launcher (#16365) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jirka <[email protected]> * Fix min-epochs and early-stopping triggering too many validation runs (#16719) Co-authored-by: Jirka Borovec <[email protected]> * Update hydra-core requirement from <1.3.0,>=1.0.5 to >=1.0.5,<1.4.0 in /requirements (#16736) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * [App] Add support for private data (#16738) Co-authored-by: thomas <[email protected]> * [App] Add rm one level below project level (#16740) Co-authored-by: Ethan Harris <[email protected]> Co-authored-by: Justus Schock <[email protected]> Co-authored-by: thomas <[email protected]> * ci: cleaning caches (#16752) * CI: Update colossalai version (#16747) Co-authored-by: Carlos Mocholí <[email protected]> type * Update version and changelog for 1.9.2 --------- Co-authored-by: Akihiro Nitta <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jirka <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: thomas chaton <[email protected]> Co-authored-by: thomas <[email protected]> Co-authored-by: Ethan Harris <[email protected]> Co-authored-by: Justus Schock <[email protected]>
What does this PR do?
Fixes a bug in the validation of strategy input.
To reproduce:
Output:
This came from user feedback on Slack. The user accidentally imported DeepSpeedStrategy from fabric and passed it to the Trainer. This is not allowed. In general, an invalid non-string type passed to the connector is triggering a code path that should never be reached, resulting in the misleading attribute error.
This PR fixes the issue in both Fabric and Trainer, and aligns both implementations in the type validation.
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
I made sure I had fun coding 🙃
cc @Borda @carmocca @justusschock @awaelchli