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.

Sign up for GitHub

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

Jump to bottom

avoid extra copies in batchnorm inference by introducing a new op, _native_batch_norm_legit_no_training #94946

Closed
wants to merge 4 commits into from
Closed

avoid extra copies in batchnorm inference by introducing a new op, _native_batch_norm_legit_no_training #94946

wants to merge 4 commits into from
+81 −6

Conversation

bdhirsh
Copy link
Contributor

@bdhirsh bdhirsh commented Feb 15, 2023 •
edited
Loading

@bdhirsh
avoid extra copies in batchnorm inference by introducing a new op, _n…
09cc979 
…ative_batch_norm_legit_no_training

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Feb 15, 2023 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/94946

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit cc46919:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

ezyang
ezyang approved these changes Feb 15, 2023
View reviewed changes
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

this is horrible but I am OK with getting it in for release. Is someone signed up for batch norm consolidation

@bdhirsh
Copy link
Contributor Author

bdhirsh commented Feb 15, 2023

Right now nobody is signed up (cc @albanD @janeyx99 ?)

@bdhirsh bdhirsh added release notes: python_frontend python frontend release notes category topic: not user facing topic category labels Feb 16, 2023
@bdhirsh
Update on "avoid extra copies in batchnorm inference by introducing a…
48124e5 
… new op, _native_batch_norm_legit_no_training"

[ghstack-poisoned]
@bdhirsh
Copy link
Contributor Author

bdhirsh commented Feb 16, 2023

added a test

bdhirsh added a commit that referenced this pull request Feb 16, 2023
@bdhirsh
avoid extra copies in batchnorm inference by introducing a new op, _n…
e4c5bdd 
…ative_batch_norm_legit_no_training

ghstack-source-id: 158da4bfbd9fc40a50282c00cda4981e1c30078e
Pull Request resolved: #94946
@bdhirsh
Copy link
Contributor Author

bdhirsh commented Feb 16, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 16, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 3 mandatory check(s) failed (Rule superuser). The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

ngimel
ngimel reviewed Feb 16, 2023
View reviewed changes
aten/src/ATen/native/Normalization.cpp Outdated
std::tuple<Tensor, Tensor, Tensor> _batch_norm_legit_no_training(
const Tensor& self, const c10::optional<Tensor>& weight_opt, const c10::optional<Tensor>& bias_opt,
const Tensor& running_mean, const Tensor& running_var, double momentum, double eps) {
return batch_norm_cpu(self, weight_opt, bias_opt, const_cast<Tensor&>(running_mean), const_cast<Tensor&>(running_var), /*train=*/false, momentum, eps);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this cpu only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops 😅 will fix when i get back to my laptop.

although this code path will never actually be hit in the PT2 workflow (since the decomp for this new op is automatically opted into by inductor + any backends that run “core aten decomps”)

@bdhirsh
Update on "avoid extra copies in batchnorm inference by introducing a…
b707d13 
… new op, _native_batch_norm_legit_no_training"

[ghstack-poisoned]
@bdhirsh
Copy link
Contributor Author

bdhirsh commented Feb 16, 2023

The extra .to(..., copy=True)s will live to see another day (I had to leave them in to appease tests, although I confirmed that copies still don't show up in the batchnorm inference graph). I think they'll be easier to clean up once batch norm consolidation happens. I'm pretty sure this is failing because there's a test passing train=False directly to _native_batch_norm_legit.default, even though that op should never handle the train=False case anymore.

@bdhirsh
Copy link
Contributor Author

bdhirsh commented Feb 16, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 2 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job Failing merge rule: Core Maintainers

@bdhirsh
Update on "avoid extra copies in batchnorm inference by introducing a…
cc46919 
… new op, _native_batch_norm_legit_no_training"

[ghstack-poisoned]
bdhirsh added a commit that referenced this pull request Feb 16, 2023
@bdhirsh
avoid extra copies in batchnorm inference by introducing a new op, _n…
328894f 
…ative_batch_norm_legit_no_training

ghstack-source-id: 83baffae010d27b4ced28189def1afbe47db2198
Pull Request resolved: #94946
@bdhirsh
Copy link
Contributor Author

bdhirsh commented Feb 16, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

This was referenced Feb 17, 2023
Closed
Closed
@msaroufim msaroufim mentioned this pull request Mar 3, 2023
Closed
pruthvistony added a commit to ROCm/pytorch that referenced this pull request May 2, 2023
@pruthvistony
Revert "avoid extra copies in batchnorm inference by introducing a ne…
6051ba4 
…w op, _native_batch_norm_legit_no_training (pytorch#94946)"

This reverts commit 68600fc.
@facebook-github-bot facebook-github-bot deleted the gh/bdhirsh/378/head branch June 8, 2023 15:46
jhavukainen pushed a commit to kulinseth/pytorch that referenced this pull request Mar 15, 2024
@bdhirsh @jhavukainen
avoid extra copies in batchnorm inference by introducing a new op, _n…
66291a5 
…ative_batch_norm_legit_no_training (pytorch#94946)

Pull Request resolved: pytorch#94946
Approved by: https://github.com/ezyang
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ngimel ngimel ngimel left review comments

@ezyang ezyang ezyang approved these changes

@albanD albanD Awaiting requested review from albanD

@antoniojkim antoniojkim Awaiting requested review from antoniojkim

@Chillee Chillee Awaiting requested review from Chillee

@jbschlosser jbschlosser Awaiting requested review from jbschlosser

@miladm miladm Awaiting requested review from miladm

@SherlockNoMad SherlockNoMad Awaiting requested review from SherlockNoMad

@voznesenskym voznesenskym Awaiting requested review from voznesenskym

@wconstab wconstab Awaiting requested review from wconstab

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: python_frontend python frontend release notes category topic: not user facing topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bdhirsh @pytorchmergebot @ezyang @ngimel