-
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
Enable inference mode for testing and predicting #8813
Enable inference mode for testing and predicting #8813
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8813 +/- ##
=======================================
- Coverage 92% 88% -4%
=======================================
Files 179 179
Lines 14910 14908 -2
=======================================
- Hits 13760 13145 -615
- Misses 1150 1763 +613 |
@tangbinh thanks for working on this! It'd be awesome to include information on performance speedups for some candidate models as well |
Should we allow the users to opt out of the inference mode if they want to? I imagine some cases where the gradients are needed at test time (e.g. higher-order derivatives), and being able to turn it off by a flag might be important. I'm think of having |
Do you have suggestions for the candidate models and some datasets to test them on? I was running some simple tasks and didn't notice much of a difference. |
@Borda do you recommend the models used for the parity benchmark? |
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.
For the 2 cases where no_grad
has been replaced already, we don't need to check the TrainerFn
because by design, they will only be called during trainer.validate()
, trainer.test()
, or trainer.predict()
. On these, no grad operations should take place so we are safe.
This is different from the following, which is the validation part of trainer.fit
. This is where I'm not sure if no_grad
should be kept:
@ananthsub @tchaton @awaelchli @carmocca Please see this notebook for some performance tests. It looks like for some common models and datasets, enabling inference mode doesn't make much of a difference in terms of inference time. The observation is consistent on both GPU and CPU. That said, I think it doesn't hurt if we decide to move forward with |
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.
thanks for working on this @tangbinh !
Dear @tangbinh @ananthsub, Could we get the benchmark for some traditional models to validate this doesn't bring a regression for Lightning performances + updated docs. Best, |
Would you mind proposing some traditional models that you have in mind? I've done some benchmark tests, although they don't follow previous examples as I couldn't find any good one. |
What does this PR do?
Does your PR introduce any breaking changes? If yes, please list them.
None
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 🙃