Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable inference mode for evaluation #12715

Merged
merged 8 commits into from Apr 21, 2022
Merged

Enable inference mode for evaluation #12715

merged 8 commits into from Apr 21, 2022

Conversation

rohitgr7
Copy link
Contributor

@rohitgr7 rohitgr7 commented Apr 11, 2022 •
edited by github-actions bot

What does this PR do?

Fixes #11530
Fixes #11018

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

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:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 馃檭

cc @Borda @justusschock

@rohitgr7 rohitgr7 added the feature Is an improvement or enhancement label Apr 11, 2022
@rohitgr7 rohitgr7 added this to the 1.7 milestone Apr 11, 2022
@rohitgr7 rohitgr7 marked this pull request as ready for review April 12, 2022 08:12
carmocca
carmocca approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@carmocca carmocca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
@ananthsub
Copy link
Contributor

We previously had support for this but needed to revert it: #8499
#9443

have you confirmed this is safe to roll out now?

@mergify mergify bot removed the has conflicts label Apr 13, 2022
@rohitgr7
Copy link
Contributor Author

We previously had support for this but needed to revert it: #8499 #9443

have you confirmed this is safe to roll out now?

we have extensive tests that seem to be passing with this change, so a good indicator that this is safe. I have also confirmed that it's working with the example shared in the issue that caused the revert last time. But even if it still has some issues, we should explore the issue more in-depth rather than reverting it just like last time. I couldn't find anything related in the PyTorch docs that some backend like gloo is not supported or any other limitation. So I think it's safe to roll out now and explore the issue more next time if any comes up.

krshrimali
krshrimali approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@krshrimali krshrimali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 馃殌

@mergify mergify bot added the ready PRs ready to be merged label Apr 13, 2022
akihironitta
akihironitta approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 馃殌

CHANGELOG.md Outdated Show resolved Hide resolved
@rohitgr7 @akihironitta
Update CHANGELOG.md
e306750 
Co-authored-by: Akihiro Nitta <nitta@akihironitta.com>
@Borda Borda enabled auto-merge (squash) April 18, 2022 18:12
awaelchli
awaelchli approved these changes Apr 20, 2022
View reviewed changes
pytorch_lightning/trainer/trainer.py
def _evaluation_context() -> Generator:
context_manager_class = (
torch.inference_mode
if _TORCH_GREATER_EQUAL_1_9 and not (dist.is_initialized() and dist.get_backend() == "gloo")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't find any information in docs or anywhere about inference_mode not being compatible with the gloo backend. A comment here in the code would probably be appropriate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding the comment. i'm definitely not satisfied still, and will investigate. this is very sus, especially since I cannot find any open or closed issue ticket on the pytorch github.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find it either. Let me open an issue on PT GitHub.

@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Apr 21, 2022
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Apr 21, 2022
@Borda Borda merged commit 4df546a into master Apr 21, 2022
@Borda Borda deleted the feat/inference_mode branch April 21, 2022 11:37
akihironitta added a commit that referenced this pull request Apr 22, 2022
@akihironitta
Revert "Enable inference mode for evaluation (#12715)"
a3da197 
This reverts commit 4df546a.
akihironitta added a commit that referenced this pull request Apr 22, 2022
@akihironitta
Revert "Revert "Enable inference mode for evaluation (#12715)""
21b57a9 
This reverts commit a3da197.
@jerome-habana jerome-habana mentioned this pull request May 9, 2022
Merged
12 tasks
@carmocca carmocca mentioned this pull request May 10, 2022
Merged
@carmocca carmocca mentioned this pull request Aug 19, 2022
Closed
@carmocca carmocca added the breaking change Includes a breaking change label Aug 19, 2022
@LouChao98 LouChao98 mentioned this pull request Sep 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awaelchli awaelchli awaelchli approved these changes

@Borda Borda Borda approved these changes

@carmocca carmocca carmocca approved these changes

@krshrimali krshrimali krshrimali approved these changes

@akihironitta akihironitta akihironitta approved these changes

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@justusschock justusschock Awaiting requested review from justusschock

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

Assignees
No one assigned
Labels
breaking change Includes a breaking change feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.7
Development

Successfully merging this pull request may close these issues.

inference_mode use torch.inference_mode() in Trainer.predict
7 participants
@rohitgr7 @ananthsub @awaelchli @Borda @carmocca @krshrimali @akihironitta