-
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
Update core/lightning.py
to core/module.py
#12740
Update core/lightning.py
to core/module.py
#12740
Conversation
Hey @tanmoyio thank you for working on this. Already looks pretty good. What's mainly missing:
|
should it be done in a follow-up? else git diff might be huge here. |
cf3e9b9
to
be9f19b
Compare
@justusschock @rohitgr7 is it looking good now? |
@tanmoyio Sorry for the delay! Could you merge or rebase master? |
@carmocca Do we need to split this PR into two as suggested by @rohitgr7? #12740 (comment) |
I would say yes. For such a large change, we need to split moving code from modifying code |
@akihironitta I have updated, let me know if I need to change anything else |
0093ba3
to
75a7c58
Compare
75a7c58
to
c9dd894
Compare
c9dd894
to
ae0dbaf
Compare
Gatekeeper merge |
What does this PR do?
Fixes #10499
Does your PR introduce any breaking changes? If yes, please list them.
While running
make test
I got some CUDA errors, I think that's issue from my side and some of the tests neednvidia/apex
,deepspeed
etc which I don't have in my system.result:
some of the errors I got
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:
Did you have fun?
Make sure you had fun coding 🙃