-
Notifications
You must be signed in to change notification settings - Fork 3.6k
FabricModule: wrap forward methods instead of monkeypatch-based redirect #20711
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
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
seems the falling stand-alone test is a false alarm :) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20711 +/- ##
=========================================
- Coverage 87% 81% -6%
=========================================
Files 268 268
Lines 23354 23354
=========================================
- Hits 20346 18913 -1433
- Misses 3008 4441 +1433 |
Ah! looks like I need to update the redirection tests. Will try to get around to it soon. Currently using this fix in prod successfully |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://lightning.ai/docs/pytorch/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Discord. Thank you for your contributions. |
Hi @tonyf , |
What does this PR do?
When running with
torch.compile
andModelParallelStrategy
the monkeypatch method of redirecting forward methods through the FabricModule's custom forward method raises an exception if the marked method does not have the same signature as the original forward method.Instead of monkeypatching forward to the method being called, this PR wraps the function to ensure the right strategy contexts are provided.
This could be cleaned up further by just wrapping the functions in
mark_forward_method
but that would require touching_LIGHTNING_MODULE_STEP_METHODS = ("training_step", "validation_step", "test_step", "predict_step")
.Fixes #20710
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--20711.org.readthedocs.build/en/20711/