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

[Quant] Use finite hops to check if the quant nodes are connected with producer #108572

Closed
wants to merge 5 commits into from
Closed

[Quant] Use finite hops to check if the quant nodes are connected with producer #108572

wants to merge 5 commits into from

Conversation

kimishpatel
Copy link
Contributor

@kimishpatel kimishpatel commented Sep 5, 2023 •
edited

Stack from ghstack (oldest at bottom):

Summary:
Using dfs to check if two nodes are connecgted is making it very slow. Even BFS was the same. So resorted to finite hop checks starting from dq node to find a node that is not inserted by quant workflow.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D48971710

@kimishpatel
[Quant] Move to BFS instead of DFS to check for connectedness
a157a8f 
Summary:
Using dfs to check if two nodes are connecgted is making it very slow.
Use of BFS makes it much faster.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Sep 5, 2023 •
edited

🔗 Helpful Links

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

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

✅ No Failures

As of commit 369b403 with merge base 0a8296d (image):
💚 Looks good so far! There are no failures yet. 💚

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

kimishpatel added a commit that referenced this pull request Sep 5, 2023
@kimishpatel
[Quant] Move to BFS instead of DFS to check for connectedness
d35a250 
Summary:
Using dfs to check if two nodes are connecgted is making it very slow.
Use of BFS makes it much faster.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: b21854ed9d950903f7964a053fdbbb95ae16b213
Pull Request resolved: #108572
@github-actions github-actions bot added the release notes: quantization release notes category label Sep 5, 2023
@kimishpatel
Copy link
Contributor Author

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

@kimishpatel kimishpatel mentioned this pull request Sep 5, 2023
Closed
@jerryzh168
Copy link
Contributor

thanks, please add a comment/doc block to talk about the cleanup plans as well so that we don't forget

@kimishpatel
Update on "[Quant] Move to BFS instead of DFS to check for connectedn…
4afca26 
…ess"

Summary:
Using dfs to check if two nodes are connecgted is making it very slow.
Use of BFS makes it much faster.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
kimishpatel added a commit that referenced this pull request Sep 5, 2023
@kimishpatel
[Quant] Move to BFS instead of DFS to check for connectedness
585e589 
Summary:
Using dfs to check if two nodes are connecgted is making it very slow.
Use of BFS makes it much faster.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: bb020ae06cefba115732ccf9c1f32e29f8d9165c
Pull Request resolved: #108572
@kimishpatel
Copy link
Contributor Author

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

@kimishpatel
Update on "[Quant] Move to BFS instead of DFS to check for connectedn…
3daeb0b 
…ess"

Summary:
Using dfs to check if two nodes are connecgted is making it very slow.
Use of BFS makes it much faster.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
kimishpatel added a commit that referenced this pull request Sep 5, 2023
@kimishpatel
[Quant] Move to BFS instead of DFS to check for connectedness
90284ce 
Summary:
Using dfs to check if two nodes are connecgted is making it very slow.
Use of BFS makes it much faster.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: 4fc054a78327ee7e57b4d681c969a780c65eee18
Pull Request resolved: #108572
@kimishpatel
Copy link
Contributor Author

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

@leslie-fang-intel
Copy link
Collaborator

Thanks for the fix.

leslie-fang-intel
leslie-fang-intel requested changes Sep 6, 2023
View reviewed changes
Copy link
Collaborator

@leslie-fang-intel leslie-fang-intel left a comment

Choose a reason for hiding this comment

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

oh... seems still has issue when running resnet152: https://gist.github.com/leslie-fang-intel/5783540cd11e9c132630319bde16a422

@jerryzh168
Copy link
Contributor

@kimishpatel maybe we have to go with traversing the hardcoded q/dq path

@kimishpatel
Update on "[Quant] Move to BFS instead of DFS to check for connectedn…
65af5ed 
…ess"

Summary:
Using dfs to check if two nodes are connecgted is making it very slow.
Use of BFS makes it much faster.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
kimishpatel added a commit that referenced this pull request Sep 6, 2023
@kimishpatel
[Quant] Move to BFS instead of DFS to check for connectedness
090f7dd 
Summary:
Using dfs to check if two nodes are connecgted is making it very slow.
Use of BFS makes it much faster.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: 2f08ec0b4e45e421976038b0f5a2e33a4b8c9820
Pull Request resolved: #108572
@kimishpatel
Copy link
Contributor Author

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

@kimishpatel
Copy link
Contributor Author

@kimishpatel maybe we have to go with traversing the hardcoded q/dq path

ok this is a step closer to hardcoding but not quite hardcoding. @jerryzh168 how do you try this on inception v4? can you copy paste command here?

@jerryzh168
Copy link
Contributor

@kimishpatel maybe we have to go with traversing the hardcoded q/dq path

ok this is a step closer to hardcoding but not quite hardcoding. @jerryzh168 how do you try this on inception v4? can you copy paste command here?

I have a diff, could you import to phabricator? I can rebase and try this out

@kimishpatel
Copy link
Contributor Author

this

it is imported to phabricator at D48971710

@kimishpatel
Update on "[Quant] Move to BFS instead of DFS to check for connectedn…
369b403 
…ess"

Summary:
Using dfs to check if two nodes are connecgted is making it very slow.
Use of BFS makes it much faster.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
kimishpatel added a commit that referenced this pull request Sep 6, 2023
@kimishpatel
[Quant] Move to BFS instead of DFS to check for connectedness
d515893 
Summary:
Using dfs to check if two nodes are connecgted is making it very slow.
Use of BFS makes it much faster.

Test Plan:
https://gist.github.com/leslie-fang-intel/9cd828623f567a3afbf41564d3546398

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: 7995027d4aad7a803a6a5308e6f4ecd80bbfb0e3
Pull Request resolved: #108572
@kimishpatel
Copy link
Contributor Author

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

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 7, 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

@kimishpatel kimishpatel changed the title [Quant] Move to BFS instead of DFS to check for connectedness [Quant] Use finite hops to check if the quant nodes are connected with producer Sep 7, 2023
@facebook-github-bot facebook-github-bot deleted the gh/kimishpatel/179/head branch September 10, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerryzh168 jerryzh168 jerryzh168 approved these changes

@leslie-fang-intel leslie-fang-intel leslie-fang-intel requested changes

@osalpekar osalpekar osalpekar approved these changes

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: AO frontend release notes: quantization release notes category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@kimishpatel @jerryzh168 @leslie-fang-intel @facebook-github-bot @pytorchmergebot @osalpekar