Update pybind to 2.6.0 #46415

malfet · 2020-10-15T20:24:17Z

Preserve PYBIND11 configuration options in torch._C._PYBIND11_COMPILER_TYPE and use them when building extensions

Also, use f-strings in torch.utils.cpp_extension

"Fixes" #46367

facebook-github-bot · 2020-10-15T20:24:31Z

💊 CI failures summary and remediations

As of commit 6f23698cbb (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).

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

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 2 times.

dr-ci · 2020-10-15T21:25:17Z

💊 CI failures summary and remediations

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

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

Extra GitHub checks: 1 failed

Failed: GitHub Actions - clang-tidy

codecov.io: 1 failed

Failed: codecov/patch

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

See how this bot performed.

This comment has been revised 79 times.

henryiii · 2020-10-16T01:09:48Z

So there are only two errors (same in two jobs, other jobs passed),

Traceback (most recent call last):
  File "test_cpp_extensions_jit.py", line 504, in test_cpp_frontend_module_python_inter_op
    verbose=True,
  File "/opt/conda/lib/python3.6/site-packages/torch/utils/cpp_extension.py", line 993, in load
    keep_intermediates=keep_intermediates)
  File "/opt/conda/lib/python3.6/site-packages/torch/utils/cpp_extension.py", line 1209, in _jit_compile
    return _import_module_from_library(name, build_directory, is_python_module)
  File "/opt/conda/lib/python3.6/site-packages/torch/utils/cpp_extension.py", line 1560, in _import_module_from_library
    return imp.load_module(module_name, file, path, description)
  File "/opt/conda/lib/python3.6/imp.py", line 243, in load_module
    return load_dynamic(name, filename, file)
  File "/opt/conda/lib/python3.6/imp.py", line 343, in load_dynamic
    return _load(spec)
  File "<frozen importlib._bootstrap>", line 684, in _load
  File "<frozen importlib._bootstrap>", line 658, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 571, in module_from_spec
  File "<frozen importlib._bootstrap_external>", line 922, in create_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
ImportError: generic_type: type "Net" referenced unknown base type "torch::nn::Module"

and

Traceback (most recent call last):
  File "test_cpp_extensions_jit.py", line 752, in test_warning
    warn_mod.foo(t, 1)
RuntimeError: Error with CPUDoubleType

Looks pybind11 related. The second one seems to be losing the "TypeError" to a more generic "RuntimeError", and the first one might be a result of some of the code correctness work? Reaching out to other devs and looking into it. The odd thing is that this only shows up on Clang?

henryiii · 2020-10-17T03:01:49Z

Interesting, the change causes macOS to fail similarly. That means the flags you are adding are being added when PyTorch is compiled, but not the JIT extension, I expect. Ideally, I think the "correct" solution would be to store the values of those two defines as added by pybind11 (which will be something like "_clang" or "_gcc" for PYBIND11_COMPILER_TYPE, and "_libcpp" or "_libstdcpp" for PYBIND11_STDLIB), and then set them to matching values via defines when compiling the JIT extension. (Assuming they really are ABI compatible, but as far as I know, I think they are, pybind11 is being extra careful here).

A test solution would be to also define them when defining the JITing. Though it's odd, it seems like all tests should fail if my JIT theory were correct; all mainline compilers get something defined here, so it should always mismatch, I'd think.

henryiii · 2020-10-17T03:12:50Z

Ah, I missed one. Can you also add PYBIND11_BUILD_ABI to an empty string too? That one was also added after the version currently there, just in a different commit. I tested it locally and that was not enough, so I think it still is my JIT idea.

malfet · 2020-10-28T04:16:24Z

I have the fix that addresses all the failures locally, will see if CI agrees with me.
But I'm still a bit uncertain, why those must be defined to empty string in the first place?

henryiii · 2020-10-28T14:24:05Z

@malfet you need to add PYBIND11_BUILD_ABI too. If you do, I think this will "pass" and prove that the only issue is the addition of these three items to the ABI string. I still think the correct fix would be to capture the values of PYBIND11_COMPILER_TYPE, PYBIND11_STDLIB, and PYBIND11_BUILD_ABI when PyTorch is compiled, and then query them and define them manually when running the JIT compile.

The old pybind11 used to build an ABI string like this:

f"__pybind11_internals_v{PYBIND11_INTERNALS_VERSION}{PYBIND11_INTERNALS_KIND}{PYBIND11_BUILD_TYPE}__"

Since 2.4, it now builds one like this:

f"__pybind11_internals_v{PYBIND11_INTERNALS_VERSION}{PYBIND11_INTERNALS_KIND}{PYBIND11_COMPILER_TYPE}{PYBIND11_STDLIB}{PYBIND11_BUILD_ABI}{PYBIND11_BUILD_TYPE}__"

This was because mixing extensions built with different compilers sometimes segfaulted, so this was the "sledgehammer" solution. Ideally, compilers that are ABI compatible (at least for the parts that pybind11 uses in it's ABI) should have the same ABI string so they can interact, but for now, this was done to avoid hard-to-debug issues until someone can carefully investigate what is compatible. If you know it works (such as is it was working before), then you can control the values of these new components.

I think in the failing test, PYBIND11_COMPILER_TYPE is "_clang", PYBIND11_STDLIB is "_libcpp", and PYBIND_BUILD_ABI is "". When compiling the JIT, then PYBIND11_COMPILER_TYPE is "_gcc", PYBIND11_STDLIB is "_libstdcxx", and PYBIND_BUILD_ABI is f"_cxxabi{__GXX_ABI_VERSION}". If it can pass if we force all these to be the same, and the easiest way to do that is to set them to the empty string, then that will prove this is the issue.

The correct fix, I believe, will be to store these three values into the pytorch extension, say as "torch._detail.pybind11_compiler_type", etc, then to define the values to match when running the JIT compile step.

torch/CMakeLists.txt

torch/utils/cpp_extension.py

ezyang · 2020-10-28T20:32:29Z

Would have been nice to have the diff without the f-string refactor

ezyang · 2020-10-28T20:33:19Z

#46415 (comment) is a really good discussion and I'd expect some reference to it in the code

ezyang

This is a horrible hack but I understand the mechanism by which it works and agree that it is appropriate to restore old behavior.

I would like it to be hella more documented.

malfet · 2020-10-28T22:19:15Z

Would have been nice to have the diff without the f-string refactor

Moved the refactors to #47025

facebook-github-bot

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

malfet · 2020-10-29T06:39:26Z

Rebased PR on top of code refactors

facebook-github-bot

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

malfet · 2020-10-29T14:53:56Z

Passing arguments with quotes via command line is non uniform on different platforms. On the other hand, I don't think there is much compiler variability on Windows

facebook-github-bot

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

henryiii · 2020-10-29T14:57:02Z

Passing arguments with quotes via command line is non uniform on different platforms

Yes, I've done it before here: https://github.com/pybind/python_example/blob/fc12654bf8915c3237ff4265e7e6889d228b5da7/src/main.cpp#L39 & https://github.com/pybind/python_example/blob/fc12654bf8915c3237ff4265e7e6889d228b5da7/setup.py#L24 but I agree, I don't think it's needed on Windows.

malfet · 2020-10-29T17:36:09Z

@henryiii Do you think it would make developers live easier if they are to be stringified on pybind11 side? (Can work on a PR if you want)

facebook-github-bot · 2020-10-29T20:22:00Z

@malfet merged this pull request in 2b6a720.

peterjc123 · 2020-11-05T02:15:04Z

torch/utils/cpp_extension.py

+
+    for pname in ["COMPILER_TYPE", "STDLIB", "BUILD_ABI"]:
+        pval = getattr(torch._C, f"_PYBIND11_{pname}")
+        if pval is not None and not IS_WINDOWS:


Would you please tell me why we add IS_WINDOWS check here?

peterjc123 · 2020-11-05T02:15:20Z

torch/utils/cpp_extension.py

+            # See note [Pybind11 ABI constants]
+            for name in ["COMPILER_TYPE", "STDLIB", "BUILD_ABI"]:
+                val = getattr(torch._C, f"_PYBIND11_{name}")
+                if val is not None and not IS_WINDOWS:


peterjc123 · 2020-11-05T02:28:11Z

Passing arguments with quotes via command line is non uniform on different platforms. On the other hand, I don't think there is much compiler variability on Windows

Well, then why don't we just don't quote all the args later on different platforms?

Summary: Recent update of pybind version from PyTorch (pytorch/pytorch#46415) created compatibility issue, resulting in mysterious errors like ``` __________________ TestRemoveException.test_remove_exceptions __________________ self = <tests.functionality.remove_exceptions_test.TestRemoveException testMethod=test_remove_exceptions> def test_remove_exceptions(self): """Test Glow's removeExceptions JIT pass""" foo_jit = torch.jit.script(foo) graph = foo_jit.graph assert graph_contains_str(graph, "prim::RaiseException") > torch_glow.removeExceptions_(graph) E TypeError: removeExceptions_(): incompatible function arguments. The following argument types are supported: E 1. (arg0: torch::jit::Graph) -> None ``` This PR aligns the pybind11 versions in glow and pytorch, hence fixing the issue. Documentation: [Optional Fixes #issue] Pull Request resolved: #5067 Test Plan: CI Reviewed By: hl475 Differential Revision: D24912990 Pulled By: yinghai fbshipit-source-id: ed22a8c70b51638fa11a5a172b93339563152869

Summary: Preserve PYBIND11 (pytorch@63ce3fb) configuration options in `torch._C._PYBIND11 (https://github.com/pytorch/pytorch/commit/63ce3fbde85672e112411ad731839e6db3c15c08)_COMPILER_TYPE` and use them when building extensions Also, use f-strings in `torch.utils.cpp_extension` "Fixes" pytorch#46367 Pull Request resolved: pytorch#46415 Reviewed By: VitalyFedyunin Differential Revision: D24605949 Pulled By: malfet fbshipit-source-id: 87340f2ed5308266a46ef8f0317316227dab9d4d (cherry picked from commit 2b6a720) Signed-off-by: Eli Uriegas <eliuriegas@fb.com>

malfet marked this pull request as draft October 15, 2020 20:24

malfet mentioned this pull request Oct 15, 2020

[chore] Test pybind11 2.6.0 (RC 2?) #46367

Open

malfet force-pushed the malfet/test-pybind-2.6.0rc2 branch 2 times, most recently from b278ebb to d2c7eea Compare October 17, 2020 01:52

malfet changed the title ~~Test pybind-2.6.0rc2~~ Test pybind-2.6.0rc3 Oct 17, 2020

henryiii mentioned this pull request Oct 17, 2020

fix: allow the ABI string to be forced pybind/pybind11#2602

Merged

malfet force-pushed the malfet/test-pybind-2.6.0rc2 branch 2 times, most recently from 0eccd32 to 2942621 Compare October 28, 2020 04:14

malfet changed the title ~~Test pybind-2.6.0rc3~~ Test pybind-2.6.0 Oct 28, 2020

henryiii reviewed Oct 28, 2020

View reviewed changes

torch/CMakeLists.txt Outdated Show resolved Hide resolved

torch/utils/cpp_extension.py Outdated Show resolved Hide resolved

torch/utils/cpp_extension.py Outdated Show resolved Hide resolved

henryiii mentioned this pull request Oct 28, 2020

CUDA Extension Build Failure on Windows and MSVC 2017 #11004

Closed

malfet changed the title ~~Test pybind-2.6.0~~ Update pybind to 2.6.0 Oct 28, 2020

malfet marked this pull request as ready for review October 28, 2020 20:26

malfet requested review from ezyang, fmassa, goldsborough and soumith as code owners October 28, 2020 20:26

wanchaol mentioned this pull request Oct 28, 2020

update pybind11 to v2.6.0 #46687

Closed

ezyang approved these changes Oct 28, 2020

View reviewed changes

malfet force-pushed the malfet/test-pybind-2.6.0rc2 branch 2 times, most recently from a6b635e to abd2827 Compare October 28, 2020 21:29

malfet force-pushed the malfet/test-pybind-2.6.0rc2 branch from abd2827 to 8fb0deb Compare October 28, 2020 22:19

facebook-github-bot reviewed Oct 28, 2020

View reviewed changes

malfet force-pushed the malfet/test-pybind-2.6.0rc2 branch from 559f97c to cb17c2f Compare October 29, 2020 06:31

malfet added 3 commits October 28, 2020 23:34

Test pybind-2.6.0

d1c498e

Preserve PYBIND constants in torch._C.

afceb1f

Document new pybind11 behaviour

f8b7375

malfet force-pushed the malfet/test-pybind-2.6.0rc2 branch from cb17c2f to f8b7375 Compare October 29, 2020 06:38

facebook-github-bot reviewed Oct 29, 2020

View reviewed changes

Do not pass pybind flags on Windows

8e3ea24

facebook-github-bot reviewed Oct 29, 2020

View reviewed changes

malfet added this to the 1.7.1 milestone Oct 29, 2020

facebook-github-bot closed this in 2b6a720 Oct 29, 2020

malfet deleted the malfet/test-pybind-2.6.0rc2 branch October 29, 2020 19:12

facebook-github-bot added the Merged label Oct 29, 2020

peterjc123 mentioned this pull request Nov 5, 2020

AttributeError: module 'torch._C' has no attribute '_PYBIND11_COMPILER_TYPE' #47333

Closed

peterjc123 reviewed Nov 5, 2020

View reviewed changes

yinghai mentioned this pull request Nov 12, 2020

update pybind pytorch/glow#5067

Closed

This was referenced Nov 20, 2020

[v1.7.1] third_party: Update pybind to point to fork #48312

Merged

[v1.7.1] Release Tracker #47622

Closed

Update pybind to 2.6.0 #46415

Update pybind to 2.6.0 #46415

Uh oh!

Conversation

malfet commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

dr-ci bot commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Extra GitHub checks: 1 failed

codecov.io: 1 failed

Uh oh!

henryiii commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

malfet commented Oct 28, 2020

Uh oh!

henryiii commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ezyang commented Oct 28, 2020

Uh oh!

ezyang commented Oct 28, 2020

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

malfet commented Oct 28, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

malfet commented Oct 29, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

malfet commented Oct 29, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

henryiii commented Oct 29, 2020

Uh oh!

malfet commented Oct 29, 2020

Uh oh!

facebook-github-bot commented Oct 29, 2020

Uh oh!

peterjc123 Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

peterjc123 Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

peterjc123 commented Nov 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

malfet commented Oct 15, 2020 •

edited

Loading

facebook-github-bot commented Oct 15, 2020 •

edited

Loading

dr-ci bot commented Oct 15, 2020 •

edited

Loading

henryiii commented Oct 16, 2020 •

edited

Loading

henryiii commented Oct 17, 2020 •

edited

Loading

henryiii commented Oct 17, 2020 •

edited

Loading

henryiii commented Oct 28, 2020 •

edited

Loading