Allow disabling custom passes. by csarofeen · Pull Request #34769 · pytorch/pytorch

csarofeen · 2020-03-14T20:10:48Z

Right now there's no way to remove a custom pass once it has been added. This PR allows that to happen, and adds an abstract class that makes it easy to enable for passes.

dr-ci · 2020-03-14T20:44:14Z

💊 CircleCI build failures summary and remediations

As of commit 42c9cc5 (more details on the Dr. CI page):

✅ None of the build failures appear to be your fault 💚

1/1 broken upstream at merge base 76d5102 since Mar 27
Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:
```
git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)
```
If your commit is older than viable/strict:
```
git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD
```
Check out the recency history of this "viable master" tracking branch.

🚧 1 upstream failure:

These were probably caused by upstream breakages:

pytorch_linux_backward_compatibility_check_test since Mar 27

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

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 70 times.

csarofeen · 2020-03-15T19:05:07Z

Included the renaming from #33674

ZolotukhinM

Thanks for adding this, I think it's a really useful feature to have! Could you please elaborate more on the implementation details? I feel it's a little hard to follow at them moment (or maybe harder than it could be). Maybe it is possible to simplify it a bit, or at least add some comments explaining why it is done this way?

ZolotukhinM · 2020-03-16T17:50:37Z

Nit: a space is missing before '{'. Do you mind running clang-format on the PR?

ZolotukhinM · 2020-03-16T17:58:59Z

Why does it have to be a struct rather than just a function?

I don't believe it does, was keeping it consistent with how it was. I wasn't wasn't sure if it was a struct for any particular reason before.

struct TORCH_API RegisterPreFusionPass { RegisterPreFusionPass(GraphPass p); }

is where we started from. I couldn't see a technical reason for it immediately but assumed there was one. If you'd like I can try to remove the structs.

ZolotukhinM · 2020-03-16T18:00:44Z

I'm not sure I quite understand why what's the purpose of the flip here. Could you please elaborate on it?

ZolotukhinM · 2020-03-16T18:59:37Z

What does this do?

ZolotukhinM

Mostly looks good, please find some comments inline.

ZolotukhinM · 2020-03-17T00:16:47Z

Similarly, can we make name a static member of the class? Also, a bit of bikeshedding: should it be called pass_id (when I first read the code and saw "name" I expected it to be a string)?

name (now pass id) and isRegistered could be a member of the class, but that would require those deriving from the class to instantiate them in their .cpp file. There's two options here, either have additional small complexity in everyone using the pass manager (defining MyPassRegister::pass_id = 0 and MyPassRegister::is_registered = false, or have them as static vars in a function. I chose this way as now to use this register function you only have to modify your header file in a pretty concise manner.

I do prefer this way, but if you have a strong preference in having the instantiations in the .cpp and static member vars, I can do that.

ZolotukhinM · 2020-03-17T00:17:57Z

Can we make isRegistered a static member of this class and add getters/setters for it instead of one function that does both?

ZolotukhinM · 2020-03-17T00:20:36Z

I kept registerPass when added registerPostPass/registerPrePass to not break it for users who might have been using the old API. This PR changes its signature anyway, so we can remove registerPass altogether, no reason to keep it.

ZolotukhinM · 2020-03-17T00:22:03Z

You might want to expose RegisterCudaFuseGraph::clearPass as well.

I need to fix merge conflicts in the big fuser PR. I'll do this then, so I can also test it. Let me know if you prefer it here.

ZolotukhinM · 2020-03-17T00:26:03Z

Could we allow registering both pre and post passes here?

I couldn't think of an easy way to do this. One option could be another bool template parameter that marks post or pre. Any thoughts?

csarofeen · 2020-03-17T19:02:44Z

Failures look unrelated to the pr

csarofeen · 2020-03-17T21:11:17Z

whoops, wrong PR, sorry.

facebook-github-bot

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

facebook-github-bot

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

facebook-github-bot

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

ZolotukhinM · 2020-03-28T02:46:33Z

@pytorchbot retest this please

facebook-github-bot

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

…gistration.

facebook-github-bot

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

csarofeen · 2020-04-02T20:15:40Z

Merged as part of #34785

csarofeen requested a review from apaszke as a code owner March 14, 2020 20:10

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Mar 14, 2020

pytorchbot added the open source label Mar 14, 2020

ZolotukhinM reviewed Mar 16, 2020

View reviewed changes

ZolotukhinM reviewed Mar 17, 2020

View reviewed changes

csarofeen added a commit to csarofeen/pytorch that referenced this pull request Mar 17, 2020

Grab: Allow disabling custom passes. pytorch#34769

1f09aea

csarofeen mentioned this pull request Mar 19, 2020

Infrastructure for a new CUDA Fuser #34785

Closed

ZolotukhinM mentioned this pull request Mar 20, 2020

Rename {Post,Pre}FusionPasses to {Post,Pre}Passes. #33674

Closed

yf225 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Mar 20, 2020

facebook-github-bot reviewed Mar 27, 2020

View reviewed changes

facebook-github-bot reviewed Mar 28, 2020

View reviewed changes

csarofeen added 12 commits March 28, 2020 16:12

Add helper to register as well as remove registered custom passes.

a238a85

Minor whitespace.

13b5ee3

Rename {Pre,Post}FusionPass -> {Pre,Post}Pass.

84c3fea

clang-format

b8000b8

Make another attempt at PassManager for custom pass registration/dere…

18d54bc

…gistration.

clang-format

6337837

Make another attempt at PassManager for custom passes.

3492a0e

Make another attempt at PassManager for custom passes.

95849d1

Strange missing symbol bug attempt 1.

380ac8a

Strange missing symbol bug attempt 2.

91f57dc

Renaming, remove registerPass.

daabc28

Remove registerPass, fix name mismatch.

fe71162

csarofeen added 3 commits March 28, 2020 16:14

Update based on CudaFuser branch, reintroduce legacy call.

21f3586

Small syntax error.

4d6bfab

Reset test_misc as we included legacy registration of passes.

000a8af

csarofeen force-pushed the customPass branch from f246647 to 000a8af Compare March 28, 2020 20:15

csarofeen added 2 commits March 28, 2020 16:18

Reset test_misc.cpp

b77e341

Remove tensor expression enable call.

42c9cc5

facebook-github-bot reviewed Apr 1, 2020

View reviewed changes

soumith closed this Apr 2, 2020

csarofeen deleted the customPass branch April 17, 2020 15:57

Conversation

csarofeen commented Mar 14, 2020

Uh oh!

dr-ci Bot commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CircleCI build failures summary and remediations

🚧 1 upstream failure:

Uh oh!

csarofeen commented Mar 15, 2020

Uh oh!

ZolotukhinM left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZolotukhinM left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csarofeen commented Mar 17, 2020

Uh oh!

csarofeen commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ZolotukhinM commented Mar 28, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

csarofeen commented Apr 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dr-ci Bot commented Mar 14, 2020 •

edited

Loading

csarofeen commented Mar 17, 2020 •

edited

Loading