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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create subgraph rewriter #49540

Closed
wants to merge 6 commits into from

Conversation

ansley
Copy link

@ansley ansley commented Dec 17, 2020

Stack from ghstack:

Differential Revision: D25869707

[ghstack-poisoned]
ansley pushed a commit that referenced this pull request Dec 17, 2020
ghstack-source-id: 2e7647351323b689634a1be1f2b85a0f7b02722e
Pull Request resolved: #49540
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Dec 17, 2020

💊 CI failures summary and remediations

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


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

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 to the (internal) Dr. CI Users group.

This comment has been revised 30 times.

ansley pushed a commit that referenced this pull request Dec 17, 2020
ghstack-source-id: b0d6e3429a8f7860102b7735f99e01aa30aeba98
Pull Request resolved: #49540
@ansley ansley linked an issue Dec 17, 2020 that may be closed by this pull request
Copy link
Collaborator

@jamesr66a jamesr66a left a comment

Choose a reason for hiding this comment

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

OK, I suggest going through the tests and making sure that they're testing pertinent parts of the algorithm. Most of them seem to end up with a no-op pattern or patterns that are untestable by comparing output values. Additionally, when i looked at the output module code, some of them seemed to have a lot of duplicate code.

I think the thing to do primarily for this PR is to try to simplify things as much as possible. I added some notes about how the replacement algorithm can be refined. Hopefully that will make things simpler and make the tests easier to write

torch/fx/node.py Outdated Show resolved Hide resolved
torch/fx/subgraph_rewriter.py Outdated Show resolved Hide resolved
torch/fx/subgraph_rewriter.py Outdated Show resolved Hide resolved
torch/fx/subgraph_rewriter.py Outdated Show resolved Hide resolved
torch/fx/subgraph_rewriter.py Outdated Show resolved Hide resolved
test/fx/test_subgraph_rewriter.py Outdated Show resolved Hide resolved
test/fx/test_subgraph_rewriter.py Outdated Show resolved Hide resolved
test/fx/test_subgraph_rewriter.py Show resolved Hide resolved
torch/fx/subgraph_rewriter.py Outdated Show resolved Hide resolved
torch/fx/subgraph_rewriter.py Outdated Show resolved Hide resolved
ansley pushed a commit that referenced this pull request Dec 28, 2020
ghstack-source-id: 221e3b493fff62ec1d229b45ce00c62ed672a2a6
Pull Request resolved: #49540
Copy link
Collaborator

@jamesr66a jamesr66a left a comment

Choose a reason for hiding this comment

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

Looking pretty good! Just some small stuff left

torch/fx/graph.py Outdated Show resolved Hide resolved
torch/fx/node.py Outdated Show resolved Hide resolved
test/fx/test_subgraph_rewriter.py Show resolved Hide resolved
test/fx/test_subgraph_rewriter.py Show resolved Hide resolved
test/fx/test_subgraph_rewriter.py Show resolved Hide resolved
torch/fx/subgraph_rewriter.py Outdated Show resolved Hide resolved
torch/fx/subgraph_rewriter.py Outdated Show resolved Hide resolved
torch/fx/subgraph_rewriter.py Outdated Show resolved Hide resolved
torch/fx/subgraph_rewriter.py Outdated Show resolved Hide resolved
ansley pushed a commit that referenced this pull request Jan 7, 2021
ghstack-source-id: 0136b617b32a9849e1c29d57836d311ce85ee677
Pull Request resolved: #49540
@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #49540 (f8aa69f) into gh/ansleyadelaide/24/base (eb87686) will decrease coverage by 0.13%.
The diff coverage is 95.55%.

@@                      Coverage Diff                      @@
##           gh/ansleyadelaide/24/base   #49540      +/-   ##
=============================================================
- Coverage                      80.70%   80.56%   -0.14%     
=============================================================
  Files                           1904     1905       +1     
  Lines                         206598   206688      +90     
=============================================================
- Hits                          166741   166527     -214     
- Misses                         39857    40161     +304     

ansley pushed a commit that referenced this pull request Jan 7, 2021
ghstack-source-id: 392ba8712b443f2c34353d9c6486a94b2ec92a62
Pull Request resolved: #49540
ansley pushed a commit that referenced this pull request Jan 11, 2021
ghstack-source-id: 9c6a0a70c4552fd9a02a8a86e98e3fa8a12e32fc
Pull Request resolved: #49540
@facebook-github-bot
Copy link
Contributor

@ansley merged this pull request in 4c97ef8.

@facebook-github-bot facebook-github-bot deleted the gh/ansleyadelaide/24/head branch January 16, 2021 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FX] Make a subgraph rewriter API
3 participants