Skip to content

[PyTorch] Remove unnecessary dispatcher.h include in torch/library.h#50317

Closed
swolchok wants to merge 4 commits intogh/swolchok/77/basefrom
gh/swolchok/77/head
Closed

[PyTorch] Remove unnecessary dispatcher.h include in torch/library.h#50317
swolchok wants to merge 4 commits intogh/swolchok/77/basefrom
gh/swolchok/77/head

Conversation

@swolchok
Copy link
Copy Markdown
Contributor

@swolchok swolchok commented Jan 9, 2021 •
edited
Loading

Stack from ghstack:

It's unused.

Differential Revision: D25859010

@swolchok swolchok mentioned this pull request Jan 9, 2021
Closed
This was referenced Jan 9, 2021
Closed
Closed
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jan 9, 2021 •
edited
Loading

💊 CI failures summary and remediations

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

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

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See CircleCI build pytorch_linux_xenial_py3_6_gcc5_4_test (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun)

Jan 26 22:55:55 AssertionError: mypy failed: torch/fx/experimental/accelerated_graph_module.py:4: error: Cannot find implementation or library stub for module named 'glow.fb.fx_glow_binding.fx_glow' [import] 
Jan 26 22:54:39 Test results will be stored in test-reports/python-unittest
Jan 26 22:54:47   test_doc_examples (__main__.TestTypeHints) ... ok (7.483s)
Jan 26 22:55:55   test_run_mypy (__main__.TestTypeHints) ... FAIL (68.378s)
Jan 26 22:55:55 
Jan 26 22:55:55 ======================================================================
Jan 26 22:55:55 FAIL [68.378s]: test_run_mypy (__main__.TestTypeHints) [mypy.ini]
Jan 26 22:55:55 ----------------------------------------------------------------------
Jan 26 22:55:55 Traceback (most recent call last):
Jan 26 22:55:55   File "test_type_hints.py", line 171, in test_run_mypy
Jan 26 22:55:55     self.fail(f"mypy failed: {stdout} {stderr}")
Jan 26 22:55:55 AssertionError: mypy failed: torch/fx/experimental/accelerated_graph_module.py:4: error: Cannot find implementation or library stub for module named 'glow.fb.fx_glow_binding.fx_glow'  [import]
Jan 26 22:55:55 torch/fx/experimental/accelerated_graph_module.py:4: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
Jan 26 22:55:55 torch/fx/experimental/accelerated_graph_module.py:4: error: Cannot find implementation or library stub for module named 'glow'  [import]
Jan 26 22:55:55 torch/fx/experimental/accelerated_graph_module.py:4: error: Cannot find implementation or library stub for module named 'glow.fb'  [import]
Jan 26 22:55:55 torch/fx/experimental/accelerated_graph_module.py:4: error: Cannot find implementation or library stub for module named 'glow.fb.fx_glow_binding'  [import]
Jan 26 22:55:55 Found 4 errors in 1 file (checked 1212 source files)
Jan 26 22:55:55  
Jan 26 22:55:55 
Jan 26 22:55:55 ----------------------------------------------------------------------
Jan 26 22:55:55 Ran 2 tests in 75.861s
Jan 26 22:55:55
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.

swolchok added a commit that referenced this pull request Jan 9, 2021
@swolchok
[PyTorch] Remove unnecessary dispatcher.h include in torch/library.h
5a16ea1 
It's unused.

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

ghstack-source-id: 119632046
Pull Request resolved: #50317
@swolchok swolchok requested review from ailzhang, bhosmer and smessmer and removed request for ailzhang and bhosmer January 11, 2021 18:49
@bhosmer
Copy link
Copy Markdown

bhosmer commented Jan 13, 2021

not sure about the test failures though

bhosmer
bhosmer reviewed Jan 13, 2021
View reviewed changes
Copy link
Copy Markdown

@bhosmer bhosmer left a comment

Choose a reason for hiding this comment

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

At least some of these failures are from client code that was depending on definitions from dispatcher.h imported transitively through e.g. library.h - not sure if you want to add the necessary dispatcher.h includes directly to the clients or forget this one; should be simple to do the former

swolchok added a commit that referenced this pull request Jan 13, 2021
@swolchok
[PyTorch] Remove unnecessary dispatcher.h include in torch/library.h
df3984e 
Pull Request resolved: #50317

It's unused.
ghstack-source-id: 119798797

Differential Revision: [D25859010](https://our.internmc.facebook.com/intern/diff/D25859010/)
swolchok added a commit that referenced this pull request Jan 19, 2021
@swolchok
[PyTorch] Remove unnecessary dispatcher.h include in torch/library.h
56c614b 
Pull Request resolved: #50317

It's unused.
ghstack-source-id: 119974236

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

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25859010/)!
swolchok added a commit that referenced this pull request Jan 26, 2021
@swolchok
[PyTorch] Remove unnecessary dispatcher.h include in torch/library.h
d3398f8 
Pull Request resolved: #50317

It's unused.
ghstack-source-id: 120427120

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

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25859010/)!
smessmer
smessmer approved these changes Mar 5, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@smessmer smessmer left a comment

Choose a reason for hiding this comment

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

This could be bc breaking for oss users who include torch/library.h and now need to add other includes. Likely they won't access the dispatcher directly, but there might be things that were transitively included from Dispatcher.h that they were using.

@smessmer smessmer added the module: bc-breaking Related to a BC-breaking change label Mar 5, 2021
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 13, 2022

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions Bot added the Stale label Apr 13, 2022
@github-actions github-actions Bot closed this May 13, 2022
@facebook-github-bot facebook-github-bot deleted the gh/swolchok/77/head branch June 12, 2022 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smessmer smessmer smessmer approved these changes

@ailzhang ailzhang Awaiting requested review from ailzhang

+1 more reviewer

@bhosmer bhosmer bhosmer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla signed module: bc-breaking Related to a BC-breaking change Stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@swolchok @facebook-github-bot @bhosmer @smessmer