-
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
Fix _module_available to detect horovod.torch properly #12377
Fix _module_available to detect horovod.torch properly #12377
Conversation
c623738
to
90f0344
Compare
Thanks for the PR @JoostvDoorn |
Some of the horovod tests are failing and seem to be skipped in master. |
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.
does it also work for any other modules?
we had the for loop there on purpose...
I tested some manually, but I think this is a good point, so I added some tests in 0d79821 to confirm imports match with the flags allowing the CI to test for this. Let me know if there are any specific cases that need to be added to this, as the exact flags for which the loop is relevant in this repository isn't fully clear to me. |
@JoostvDoorn It looks like #12379 will have to be completed first, right? If you want, you can merge #12379 into this branch to see if both work together. |
…ghtning into simplify-module-checks
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.
Great!
What does this PR do?
Fixes #12370, such that horovod.torch is again properly detected.
This PR simplifies the logic of the _module_available function. I think in general it's not a good idea to try to deviate too much from importlib when testing for module availability, if the specific logic of
hasattr
is necessary for any check it would be better to handle that case separately. I tried reproducing the error that first triggered this change in the torchmetrics repo Lightning-AI/torchmetrics#770 by testing_module_available("transformers.models.auto")
and this didn't give me any problems, but a maintainer might be a better judge of this. I can replicate this code to that repo later.Note I run horovod in develop mode not sure if that's related, but latest pypi version of horovod has a very restrictive pin on the pytorch lightning version.
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃