-
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
make plugin type check more flexible #20186
Conversation
Ah, I just realized that one can pass in |
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.
Approved, completing the PR in #20452
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20186 +/- ##
=========================================
- Coverage 88% 79% -9%
=========================================
Files 267 264 -3
Lines 23274 23220 -54
=========================================
- Hits 20381 18271 -2110
- Misses 2893 4949 +2056 |
Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Luca Antiga <[email protected]>
* Add `convert_module` to FSDP * Update ChangeLog * make plugin type check more flexible (#20186) Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Luca Antiga <[email protected]> * Make plugin type check more flexible (Fabric) (#20452) * make plugin type check more flexible * Change signature and make the equivalent changes to Fabric connector --------- Co-authored-by: Jianing Yang <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> * Pin setuptools for gpu builds * Fix link in doc --------- Co-authored-by: Luca Antiga <[email protected]> Co-authored-by: Jianing Yang <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Luca Antiga <[email protected]>
What does this PR do?
The current type check of
plugins
for Trainer is restricted tolist
byisinstance(plugins, list)
. However, ifplugins
is instantiated from something like Hydra or OmegaConf, it will have type<class 'omegaconf.listconfig.ListConfig'>
, and the current logic would wrap that list-like object with a second-layer list and cause subsequant logic to fail. If you look at the code, it is not necessary thatplugins
must be a list - it suffices as long as it is iterable. Therefore, the proposed simple fix change the check fromisinstance(plugins, list)
toisinstance(plugins, Iterable)
. I hope this can benefit those who uses Hydra or OmegaConf.Before submitting
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
📚 Documentation preview 📚: https://pytorch-lightning--20186.org.readthedocs.build/en/20186/