Skip to content

[TorchTidy] Add pattern to detect if bias is enabled in conv2d followed by batchnorm2d#81941

Closed
davidchencsl wants to merge 10 commits intogh/davidchencsl/25/basefrom
gh/davidchencsl/25/head
Closed

[TorchTidy] Add pattern to detect if bias is enabled in conv2d followed by batchnorm2d#81941
davidchencsl wants to merge 10 commits intogh/davidchencsl/25/basefrom
gh/davidchencsl/25/head

Conversation

@davidchencsl
Copy link
Contributor

@davidchencsl davidchencsl commented Jul 21, 2022 •
edited
Loading

Stack from ghstack (oldest at bottom):

Summary: For cuda models only, if we have nn.Conv2d followed by nn.Batchnorm2d, we don't actually need to use bias=True in Conv2d as the math will cancel out the effect of bias

Test Plan: Added test in test_profiler.py

Differential Revision: D38082643

This was referenced Jul 21, 2022
Closed
Closed
Closed
Closed
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 21, 2022 •
edited
Loading

🔗 Helpful links

✅ No Failures (0 Pending)

As of commit 1455de3 (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

davidchencsl added a commit that referenced this pull request Jul 21, 2022
@davidchencsl
[TorchTidy] Add pattern to detect if bias is enabled in conv2d followed
3ad0bcb 
by batchnorm2d

ghstack-source-id: 1093291
Pull Request resolved: #81941
@davidchencsl davidchencsl requested a review from robieta July 21, 2022 22:55
@davidchencsl
Update on "[TorchTidy] Add pattern to detect if bias is enabled in co…
783426c 
…nv2d followed"

by batchnorm2d

[ghstack-poisoned]
@davidchencsl
Update on "[TorchTidy] Add pattern to detect if bias is enabled in co…
d74f8b0 
…nv2d followed"

by batchnorm2d

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Jul 22, 2022
@davidchencsl
[TorchTidy] Add pattern to detect if bias is enabled in conv2d followed
e0d531e 
by batchnorm2d

ghstack-source-id: a377c59
Pull Request resolved: #81941
@davidchencsl
Copy link
Contributor Author

davidchencsl commented Jul 22, 2022

@davidchencsl has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

1 similar comment
@davidchencsl
Copy link
Contributor Author

davidchencsl commented Jul 22, 2022

@davidchencsl has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@davidchencsl
Update on "[TorchTidy] Add pattern to detect if bias is enabled in co…
2edf068 
…nv2d followed"

by batchnorm2d

Differential Revision: [D38082643](https://our.internmc.facebook.com/intern/diff/D38082643)

[ghstack-poisoned]
@davidchencsl
Update on "[TorchTidy] Add pattern to detect if bias is enabled in co…
ebf9806 
…nv2d followed"

by batchnorm2d

Differential Revision: [D38082643](https://our.internmc.facebook.com/intern/diff/D38082643)

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Jul 25, 2022
@davidchencsl
[TorchTidy] Add pattern to detect if bias is enabled in conv2d followed
6bd56f6 
by batchnorm2d

ghstack-source-id: 2ad01ce
Pull Request resolved: #81941
@davidchencsl
Copy link
Contributor Author

davidchencsl commented Jul 25, 2022

@davidchencsl has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@davidchencsl
Update on "[TorchTidy] Add pattern to detect if bias is enabled in co…
25e09e8 
…nv2d followed"

by batchnorm2d

Differential Revision: [D38082643](https://our.internmc.facebook.com/intern/diff/D38082643)

[ghstack-poisoned]
@davidchencsl davidchencsl mentioned this pull request Jul 26, 2022
Closed
@davidchencsl
Copy link
Contributor Author

davidchencsl commented Jul 26, 2022

@davidchencsl has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@davidchencsl davidchencsl mentioned this pull request Jul 26, 2022
Closed
@davidchencsl
Update on "[TorchTidy] Add pattern to detect if bias is enabled in co…
bbedb94 
…nv2d followed"

by batchnorm2d

Differential Revision: [D38082643](https://our.internmc.facebook.com/intern/diff/D38082643)

[ghstack-poisoned]
@davidchencsl
Copy link
Contributor Author

davidchencsl commented Jul 27, 2022

@davidchencsl has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

robieta
robieta approved these changes Jul 27, 2022
View reviewed changes
Copy link
Contributor

@robieta robieta left a comment

Choose a reason for hiding this comment

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

LGTM

@davidchencsl davidchencsl changed the title [TorchTidy] Add pattern to detect if bias is enabled in conv2d followed [TorchTidy] Add pattern to detect if bias is enabled in conv2d followed by batchnorm2d Jul 27, 2022
@davidchencsl
Update on "[TorchTidy] Add pattern to detect if bias is enabled in co…
fb45b61 
…nv2d followed by batchnorm2d"


Summary: For cuda models only, if we have nn.Conv2d followed by nn.Batchnorm2d, we don't actually need to use bias=True in Conv2d as the math will cancel out the effect of bias

Test Plan: Added test in test_profiler.py

Differential Revision: [D38082643](https://our.internmc.facebook.com/intern/diff/D38082643)

[ghstack-poisoned]
@davidchencsl
Update on "[TorchTidy] Add pattern to detect if bias is enabled in co…
78d5d74 
…nv2d followed by batchnorm2d"


Summary: For cuda models only, if we have nn.Conv2d followed by nn.Batchnorm2d, we don't actually need to use bias=True in Conv2d as the math will cancel out the effect of bias

Test Plan: Added test in test_profiler.py

Differential Revision: [D38082643](https://our.internmc.facebook.com/intern/diff/D38082643)

[ghstack-poisoned]
@github-actions
Copy link
Contributor

github-actions bot commented Jul 28, 2022

Hey @davidchencsl.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@ZainRizvi
Copy link
Contributor

ZainRizvi commented Jul 28, 2022 •
edited
Loading

Reverting this PR since it's failing on ROCm builds

See these logs for more details: https://github.com/pytorch/pytorch/runs/7557100340?check_suite_focus=true

cc @davidchencsl

@ZainRizvi
Copy link
Contributor

ZainRizvi commented Jul 28, 2022

@pytorchmergebot revert -m "[NOSIGNAL] New test failed on ROCm builds"

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 28, 2022

❌ 🤖 pytorchbot command failed:

@pytorchbot revert: error: the following arguments are required: -c/--classification

usage: @pytorchbot revert -m MESSAGE -c
                          {nosignal,ignoredsignal,landrace,weird,ghfirst}

Try @pytorchbot --help for more info.

@ZainRizvi
Copy link
Contributor

ZainRizvi commented Jul 28, 2022

@pytorchmergebot revert -m "New test failed on ROCm builds" -c nosignal

@pytorch pytorch deleted a comment from pytorch-bot bot Jul 28, 2022
@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jul 28, 2022

@pytorchbot successfully started a revert job. Check the current status here

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jul 28, 2022

@davidchencsl your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Jul 28, 2022
@pytorchmergebot
Revert "[TorchTidy] Add pattern to detect if bias is enabled in conv2…
2fe7316 
…d followed by batchnorm2d (#81941)"

This reverts commit 615f2fd.

Reverted #81941 on behalf of https://github.com/ZainRizvi due to New test failed on ROCm builds
davidchencsl added a commit that referenced this pull request Jul 28, 2022
@davidchencsl
[TorchTidy] Backout #81941 Add pattern to detect if bias is enabled i…
471b713 
…n conv2d followed by batchnorm2d

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Jul 28, 2022
@davidchencsl
Update on "[TorchTidy] Backout #81941 Add pattern to detect if bias i…
3d1554d 
…s enabled in conv2d followed by batchnorm2d"

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Jul 28, 2022
@davidchencsl
Update base for Update on "[TorchTidy] Backout #81941 Add pattern to …
27757ed 
…detect if bias is enabled in conv2d followed by batchnorm2d"

Differential Revision: [D38254079](https://our.internmc.facebook.com/intern/diff/D38254079)

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Jul 28, 2022
@davidchencsl
Update on "[TorchTidy] Backout #81941 Add pattern to detect if bias i…
4e64279 
…s enabled in conv2d followed by batchnorm2d"

Differential Revision: [D38254079](https://our.internmc.facebook.com/intern/diff/D38254079)

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Jul 28, 2022
@davidchencsl
Update base for Update on "[TorchTidy] Backout #81941 Add pattern to …
adbd46c 
…detect if bias is enabled in conv2d followed by batchnorm2d"

Differential Revision: [D38254079](https://our.internmc.facebook.com/intern/diff/D38254079)

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Jul 28, 2022
@davidchencsl
Update on "[TorchTidy] Backout #81941 Add pattern to detect if bias i…
2ecd9d3 
…s enabled in conv2d followed by batchnorm2d"

Differential Revision: [D38254079](https://our.internmc.facebook.com/intern/diff/D38254079)

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Jul 29, 2022
@davidchencsl
Update base for Update on "[TorchTidy] Backout #81941 Add pattern to …
80eca53 
…detect if bias is enabled in conv2d followed by batchnorm2d"

Differential Revision: [D38254079](https://our.internmc.facebook.com/intern/diff/D38254079)

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Jul 29, 2022
@davidchencsl
Update on "[TorchTidy] Backout #81941 Add pattern to detect if bias i…
74a0789 
…s enabled in conv2d followed by batchnorm2d"

Differential Revision: [D38254079](https://our.internmc.facebook.com/intern/diff/D38254079)

[ghstack-poisoned]
@osalpekar osalpekar mentioned this pull request Jul 29, 2022
Closed
davidchencsl added a commit that referenced this pull request Jul 29, 2022
@davidchencsl
Update base for Update on "[TorchTidy] Reland #81941 Add pattern to d…
bbda3f2 
…etect if bias is enabled in conv2d followed by batchnorm2d"

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Jul 29, 2022
@davidchencsl
Update on "[TorchTidy] Reland #81941 Add pattern to detect if bias is…
cbcea07 
… enabled in conv2d followed by batchnorm2d"

[ghstack-poisoned]
@facebook-github-bot facebook-github-bot deleted the gh/davidchencsl/25/head branch July 31, 2022 14:18
davidchencsl added a commit that referenced this pull request Aug 2, 2022
@davidchencsl
Update base for Update on "[TorchTidy] Reland #81941 Add pattern to d…
272c185 
…etect if bias is enabled in conv2d followed by batchnorm2d"

[ghstack-poisoned]
davidchencsl added a commit that referenced this pull request Aug 2, 2022
@davidchencsl
Update on "[TorchTidy] Reland #81941 Add pattern to detect if bias is…
8011e04 
… enabled in conv2d followed by batchnorm2d"

[ghstack-poisoned]
pytorchmergebot pushed a commit that referenced this pull request Aug 2, 2022
@davidchencsl @pytorchmergebot
[TorchTidy] Reland #81941 Add pattern to detect if bias is enabled in…
ae2e303 
… conv2d followed by batchnorm2d (#82421)

Pull Request resolved: #82421
Approved by: https://github.com/robieta
ngimel
ngimel reviewed Aug 2, 2022
View reviewed changes
torch/profiler/_pattern_matcher.py
event, lambda e: e.name().startswith("nn.Module: Conv2d"))
if not event:
return False
event = self.next_of(event)
Copy link
Collaborator

@ngimel ngimel Aug 2, 2022

Choose a reason for hiding this comment

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

how would this work if forward is something like

x1=self.conv1(x)
x2=self.conv2(x)
x1=self.bn1(x1)
x2=self.bn2(x2)

Wouldn't next_of be just the next conv in this case?

Copy link
Contributor Author

@davidchencsl davidchencsl Aug 2, 2022

Choose a reason for hiding this comment

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

Ah. Right now I am going to narrow the scope of detection to Situation in nn.Sequential, or simpler scenarios, as we do not have tensor identities information (Only size and dtype) as of right now. But great point! I will add a todo

Copy link
Contributor Author

@davidchencsl davidchencsl Aug 2, 2022

Choose a reason for hiding this comment

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

But if we do have that information, we can do something like:

event = next_used_event(event.output_tensor)

Copy link
Collaborator

@ngimel ngimel Aug 2, 2022

Choose a reason for hiding this comment

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

tensor identities are needed for more robust pattern detection

facebook-github-bot pushed a commit that referenced this pull request Aug 3, 2022
@davidchencsl @facebook-github-bot
[TorchTidy] Reland #81941 Add pattern to detect if bias is enabled in…
8e9b462 
… conv2d followed by batchnorm2d (#82421) (#82421)

Summary:
Pull Request resolved: #82421
Approved by: https://github.com/robieta

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/ae2e303de0795642b0c811f356e1024285e3ac00

Reviewed By: kit1980

Differential Revision: D38359670

Pulled By: davidchencsl

fbshipit-source-id: 5f3d8555775133dd3ed48b949f037a301137ae74
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

@robieta robieta robieta approved these changes

Assignees

No one assigned

Labels

cla signed Merged Reverted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@davidchencsl @facebook-github-bot @pytorchmergebot @ZainRizvi @robieta @ngimel