[JIT] make RegisterCudaFuseGraph use TORCH_API instead of C10_EXPORT #73742
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I think this was causing multiple implementations of RegisterCudaFuseGraph on windows. It looks like both torch_cpu.dll and torch_python.dll were exporting RegisterCudaFuseGraph implementations, so RegisterCudaFuseGraph::isRegistered() would refer to a _different_ static bool depending on whether the caller was in torch_cpu.dll or torch_python.dll. See #73717 for a demonstration. [ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
|
Contributor
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 595968a (more details on the Dr. CI page): 💚 💚 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. |
facebook-github-bot
added
the
oncall: jit
…C10_EXPORT" I think this was causing multiple implementations of RegisterCudaFuseGraph on windows. It looks like both torch_cpu.dll and torch_python.dll were exporting RegisterCudaFuseGraph implementations, so RegisterCudaFuseGraph::isRegistered() would refer to a _different_ static bool depending on whether the caller was in torch_cpu.dll or torch_python.dll. See #73717 for a demonstration. [ghstack-poisoned]
Contributor
Author
|
@davidberard98 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
eellison
approved these changes
Mar 4, 2022
Contributor
eellison
left a comment
eellison
left a comment
There was a problem hiding this comment.
Should we just grep replace C10_EXPORT with TORCH_API everywhere in torch/csrc/jit ?
Collaborator
Similar question, we are also using |
Contributor
Author
|
@eellison @jjsjann123 I think some of them belong to |
davidberard98
added a commit
that referenced
this pull request
Mar 5, 2022
These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: #73742 ghstack-source-id: 9c08b52 Pull Request resolved: #73818
davidberard98
added a commit
that referenced
this pull request
Mar 5, 2022
These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: #73742 [ghstack-poisoned]
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 7, 2022
…73742) Summary: Pull Request resolved: #73742 I think this was causing multiple implementations of RegisterCudaFuseGraph on windows. It looks like both torch_cpu.dll and torch_python.dll were exporting RegisterCudaFuseGraph implementations, so RegisterCudaFuseGraph::isRegistered() would refer to a _different_ static bool depending on whether the caller was in torch_cpu.dll or torch_python.dll. See #73717 for a demonstration. Test Plan: Imported from OSS Reviewed By: eellison Differential Revision: D34618623 Pulled By: davidberard98 fbshipit-source-id: f9bc4fe792098bfabf50ecfcd1f785ed039184bd
Contributor
|
Hey @davidberard98. |
davidberard98
added a commit
that referenced
this pull request
Mar 7, 2022
These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: #73742 [ghstack-poisoned]
davidberard98
added a commit
that referenced
this pull request
Mar 7, 2022
These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: #73742 ghstack-source-id: 9ca88a0 Pull Request resolved: #73818
davidberard98
added a commit
that referenced
this pull request
Mar 8, 2022
These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: #73742 Differential Revision: [D34698175](https://our.internmc.facebook.com/intern/diff/D34698175) [ghstack-poisoned]
davidberard98
added a commit
that referenced
this pull request
Mar 8, 2022
These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: #73742 Differential Revision: [D34698175](https://our.internmc.facebook.com/intern/diff/D34698175) [ghstack-poisoned]
davidberard98
added a commit
that referenced
this pull request
Mar 8, 2022
These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: #73742 ghstack-source-id: 73592b7 Pull Request resolved: #73818
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Mar 9, 2022
…(#73742) Summary: Pull Request resolved: pytorch/pytorch#73742 I think this was causing multiple implementations of RegisterCudaFuseGraph on windows. It looks like both torch_cpu.dll and torch_python.dll were exporting RegisterCudaFuseGraph implementations, so RegisterCudaFuseGraph::isRegistered() would refer to a _different_ static bool depending on whether the caller was in torch_cpu.dll or torch_python.dll. See #73717 for a demonstration. Test Plan: Imported from OSS Reviewed By: eellison Differential Revision: D34618623 Pulled By: davidberard98 fbshipit-source-id: f9bc4fe792098bfabf50ecfcd1f785ed039184bd (cherry picked from commit 2de58679342914f456d63a9f952357ac163cc4ec)
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Mar 9, 2022
…(#73742) Summary: Pull Request resolved: pytorch/pytorch#73742 I think this was causing multiple implementations of RegisterCudaFuseGraph on windows. It looks like both torch_cpu.dll and torch_python.dll were exporting RegisterCudaFuseGraph implementations, so RegisterCudaFuseGraph::isRegistered() would refer to a _different_ static bool depending on whether the caller was in torch_cpu.dll or torch_python.dll. See #73717 for a demonstration. Test Plan: Imported from OSS Reviewed By: eellison Differential Revision: D34618623 Pulled By: davidberard98 fbshipit-source-id: f9bc4fe792098bfabf50ecfcd1f785ed039184bd (cherry picked from commit 2de58679342914f456d63a9f952357ac163cc4ec)
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 14, 2022
Summary: Pull Request resolved: #73818 These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: #73742 Test Plan: Imported from OSS Reviewed By: george-qi Differential Revision: D34698175 Pulled By: davidberard98 fbshipit-source-id: cb871e861cf966bff596cfa8340a32a17fca0b66
pytorchmergebot
pushed a commit
that referenced
this pull request
Mar 14, 2022
Summary: Pull Request resolved: #73818 These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: #73742 Test Plan: Imported from OSS Reviewed By: george-qi Differential Revision: D34698175 Pulled By: davidberard98 fbshipit-source-id: cb871e861cf966bff596cfa8340a32a17fca0b66 (cherry picked from commit 6b9988e)
jjsjann123
pushed a commit
to jjsjann123/nvfuser
that referenced
this pull request
Oct 29, 2022
Summary: Pull Request resolved: pytorch/pytorch#73818 These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: pytorch/pytorch#73742 Test Plan: Imported from OSS Reviewed By: george-qi Differential Revision: D34698175 Pulled By: davidberard98 fbshipit-source-id: cb871e861cf966bff596cfa8340a32a17fca0b66 (cherry picked from commit 6b9988e5688e6d4a9928c3e331efb74f000a9e4a)
jjsjann123
pushed a commit
to jjsjann123/nvfuser
that referenced
this pull request
Nov 10, 2022
Summary: Pull Request resolved: pytorch/pytorch#73818 These all appear to be defined in libtorch_cpu.so, so they should be marked with TORCH_API. TORCH_API means that these symbols are exported from libtorch_cpu.so and no other libraries. In comparison, C10_EXPORT will export the symbol in _all_ built libraries, if it's available. I think most of these were fine because most were only defined in cpp files (which would only be included in the targets for one .so file). However, the change in pass_manager.h affects behavior, since the class is defined in the .h file, which could result in two separate implementations of the same static functions. Previously we saw issues on windows with this: pytorch/pytorch#73742 Test Plan: Imported from OSS Reviewed By: george-qi Differential Revision: D34698175 Pulled By: davidberard98 fbshipit-source-id: cb871e861cf966bff596cfa8340a32a17fca0b66 (cherry picked from commit 6b9988e5688e6d4a9928c3e331efb74f000a9e4a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
I think this was causing multiple implementations of
RegisterCudaFuseGraph on windows. It looks like both torch_cpu.dll and
torch_python.dll were exporting RegisterCudaFuseGraph implementations,
so RegisterCudaFuseGraph::isRegistered() would refer to a different
static bool depending on whether the caller was in torch_cpu.dll or
torch_python.dll. See #73717 for a demonstration.
Differential Revision: D34618623