Uniformly apply Windows logic in cpp_extensions everywhere #31161

ezyang · 2019-12-12T02:49:15Z

Stack from ghstack:

Don't use RTLD_GLOBAL to load _C. #31162 Don't use RTLD_GLOBAL to load _C.
Uniformly apply Windows logic in cpp_extensions everywhere #31161 Uniformly apply Windows logic in cpp_extensions everywhere

Previously, it wasn't necessary to specify DT_NEEDED in C++ extensions on Linux (aka pass -l flags) because all of the symbols would have already been loaded with RTLD_GLOBAL, so there wouldn't be any undefined symbols. But when we switch to loading _C with RTLD_LOCAL, it's now necessary for all the C++ extensions to know what libraries to link with. The resulting code is clearer and more uniform, so it's wins all around.

Signed-off-by: Edward Z. Yang ezyang@fb.com

Differential Revision: D19262578

Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

kostmo · 2019-12-12T03:12:53Z

💊 CircleCI build failures summary and remediations

As of commit 8a6d169:

1/3 failures introduced in this PR
2/3 recognized as flaky ❄️
- Re-run these jobs?

Detailed failure analysis

One may explore the probable reasons each build failed interactively on the Dr. CI website.

❄️ 2 failures recognized as flaky

The following build failures have been detected as flaky and may not be your fault:

pytorch_macos_10_13_py3_test (1/2)

Step: "Test" (full log | pattern match details) ❄️

Jan 08 12:46:26 RuntimeError: test_quantized_nn_mods failed! Received signal: SIGSEGV

Jan 08 12:46:22 Generating XML reports... 
Jan 08 12:46:22 Running test_quantized_nn_mods ... [2020-01-08 12:46:22.980646] 
Jan 08 12:46:23  
Jan 08 12:46:23 Running tests... 
Jan 08 12:46:23 ---------------------------------------------------------------------- 
Jan 08 12:46:26 s...Traceback (most recent call last): 
Jan 08 12:46:26   File "test/run_test.py", line 456, in <module> 
Jan 08 12:46:26     main() 
Jan 08 12:46:26   File "test/run_test.py", line 449, in main 
Jan 08 12:46:26     raise RuntimeError(message) 
Jan 08 12:46:26 RuntimeError: test_quantized_nn_mods failed! Received signal: SIGSEGV 
Jan 08 12:46:26 + cleanup 
Jan 08 12:46:26 + retcode=1 
Jan 08 12:46:26 + set +x

pytorch_linux_xenial_cuda9_cudnn7_py3_NO_AVX_NO_AVX2_test (2/2)

Step: "Test" (full log | pattern match details) ❄️

Jan 08 21:07:23 AssertionError: 1024 not less than or equal to 1e-05 : __main__.TestAutogradDeviceTypeCUDA.test_logdet_1x1_cuda leaked 1024 bytes CUDA memory on device 0

Jan 08 21:07:23 ====================================================================== 
Jan 08 21:07:23 FAIL [0.118s]: test_logdet_1x1_cuda (__main__.TestAutogradDeviceTypeCUDA) 
Jan 08 21:07:23 ---------------------------------------------------------------------- 
Jan 08 21:07:23 Traceback (most recent call last): 
Jan 08 21:07:23   File "/var/lib/jenkins/workspace/test/common_utils.py", line 676, in wrapper 
Jan 08 21:07:23     method(*args, **kwargs) 
Jan 08 21:07:23   File "/var/lib/jenkins/workspace/test/common_utils.py", line 532, in __exit__ 
Jan 08 21:07:23     self.name, after - before, i)) 
Jan 08 21:07:23   File "/var/lib/jenkins/workspace/test/common_utils.py", line 888, in assertEqual 
Jan 08 21:07:23     super(TestCase, self).assertLessEqual(abs(x - y), prec, message) 
Jan 08 21:07:23 AssertionError: 1024 not less than or equal to 1e-05 : __main__.TestAutogradDeviceTypeCUDA.test_logdet_1x1_cuda leaked 1024 bytes CUDA memory on device 0 
Jan 08 21:07:23  
Jan 08 21:07:23 ---------------------------------------------------------------------- 
Jan 08 21:07:23 Ran 1885 tests in 891.537s 
Jan 08 21:07:23  
Jan 08 21:07:23 FAILED (failures=1, skipped=13, expected failures=1) 
Jan 08 21:07:23  
Jan 08 21:07:23 Generating XML reports... 
Jan 08 21:07:24 Traceback (most recent call last): 
Jan 08 21:07:24   File "test/run_test.py", line 456, in <module> 
Jan 08 21:07:24     main()

1 failure not recognized by patterns:

Job	Step	Status
^{pytorch_linux_backward_compatibility_check_test}	^Test	New in PR

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.

This comment has been revised 26 times.

Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

Previously, it wasn't necessary to specify `DT_NEEDED` in C++ extensions on Linux (aka pass `-l` flags) because all of the symbols would have already been loaded with `RTLD_GLOBAL`, so there wouldn't be any undefined symbols. But when we switch to loading `_C` with `RTLD_LOCAL`, it's now necessary for all the C++ extensions to know what libraries to link with. The resulting code is clearer and more uniform, so it's wins all around. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

Previously, it wasn't necessary to specify `DT_NEEDED` in C++ extensions on Linux (aka pass `-l` flags) because all of the symbols would have already been loaded with `RTLD_GLOBAL`, so there wouldn't be any undefined symbols. But when we switch to loading `_C` with `RTLD_LOCAL`, it's now necessary for all the C++ extensions to know what libraries to link with. The resulting code is clearer and more uniform, so it's wins all around. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D19262578](https://our.internmc.facebook.com/intern/diff/D19262578) [ghstack-poisoned]

soumith

lgtm

Previously, it wasn't necessary to specify `DT_NEEDED` in C++ extensions on Linux (aka pass `-l` flags) because all of the symbols would have already been loaded with `RTLD_GLOBAL`, so there wouldn't be any undefined symbols. But when we switch to loading `_C` with `RTLD_LOCAL`, it's now necessary for all the C++ extensions to know what libraries to link with. The resulting code is clearer and more uniform, so it's wins all around. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D19262578](https://our.internmc.facebook.com/intern/diff/D19262578) [ghstack-poisoned]

yf225 · 2020-01-08T12:57:09Z

@peterjc123 Would you like to take a look?

peterjc123 · 2020-01-08T13:12:31Z

torch/utils/cpp_extension.py

-        libraries.append('_C')
+    libraries.append('c10')
+    libraries.append('c10_cuda')
+    libraries.append('torch')


What is torch here used for? The functions should be in torch_cpu and torch_cuda, right?

Yeah, it's supposed to be an empty library. I put torch in here for "good luck", in case someone ever accidentally puts some symbols in it.

peterjc123 · 2020-01-08T13:12:43Z

torch/utils/cpp_extension.py

+
+    libraries = kwargs.get('libraries', [])
+    libraries.append('c10')
+    libraries.append('torch')


peterjc123 · 2020-01-08T13:13:45Z

torch/utils/cpp_extension.py

+        extra_ldflags.append('-ltorch_cpu')
+        if with_cuda:
+            extra_ldflags.append('-ltorch_cuda')
+        extra_ldflags.append('-ltorch')


Previously, it wasn't necessary to specify `DT_NEEDED` in C++ extensions on Linux (aka pass `-l` flags) because all of the symbols would have already been loaded with `RTLD_GLOBAL`, so there wouldn't be any undefined symbols. But when we switch to loading `_C` with `RTLD_LOCAL`, it's now necessary for all the C++ extensions to know what libraries to link with. The resulting code is clearer and more uniform, so it's wins all around. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D19262578](https://our.internmc.facebook.com/intern/diff/D19262578) [ghstack-poisoned]

facebook-github-bot · 2020-01-09T17:11:26Z

@ezyang merged this pull request in 8614860.

Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 77b5ed9aa069925703ede06a23b268084347436f Pull Request resolved: pytorch/pytorch#31161

Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 98f3dbeb0958d541e2b72e210097a42f1d10e046 Pull Request resolved: pytorch/pytorch#31161

…1161) Summary: Pull Request resolved: pytorch#31161 Previously, it wasn't necessary to specify `DT_NEEDED` in C++ extensions on Linux (aka pass `-l` flags) because all of the symbols would have already been loaded with `RTLD_GLOBAL`, so there wouldn't be any undefined symbols. But when we switch to loading `_C` with `RTLD_LOCAL`, it's now necessary for all the C++ extensions to know what libraries to link with. The resulting code is clearer and more uniform, so it's wins all around. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Test Plan: Imported from OSS Differential Revision: D19262578 Pulled By: ezyang fbshipit-source-id: a893cc96f2e9aad1c064a6de4f7ccf79257dec3f

Actually get rid of Windows hacks in cpp_extensions.

a12a58a

Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

ezyang requested review from fmassa, goldsborough and soumith as code owners December 12, 2019 02:49

Update on "Actually get rid of Windows hacks in cpp_extensions."

1a0c19b

Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

ezyang mentioned this pull request Dec 12, 2019

Pick parallel MKL implementation over sequential implementation. #31165

Closed

Update on "Actually get rid of Windows hacks in cpp_extensions."

d964297

Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

This was referenced Dec 12, 2019

Move AutogradMeta and DeviceGuardImplInterface virtual methods out-of-line. #31176

Closed

Use libmkl_rt, or statically link against MKL #31177

Closed

ezyang added 2 commits December 13, 2019 10:09

Update on "Actually get rid of Windows hacks in cpp_extensions."

9e148a1

Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

Update on "Actually get rid of Windows hacks in cpp_extensions."

cebef29

Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

ezyang mentioned this pull request Dec 13, 2019

Don't unconditionally compile runJITCPPTests #31236

Closed

Update on "Actually get rid of Windows hacks in cpp_extensions."

91d74e0

Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

ezyang changed the title ~~Actually get rid of Windows hacks in cpp_extensions.~~ Uniformly apply Windows logic in cpp_extensions everywhere Dec 14, 2019

ezyang requested a review from yf225 January 2, 2020 14:33

ezyang mentioned this pull request Jan 6, 2020

Revert "Move AutogradMeta and DeviceGuardImplInterface virtual methods out-of-line." #31899

Closed

ezyang added 2 commits January 6, 2020 20:03

soumith approved these changes Jan 7, 2020

View reviewed changes

ezyang added 2 commits January 7, 2020 10:28

yf225 requested a review from peterjc123 January 8, 2020 12:57

peterjc123 reviewed Jan 8, 2020

View reviewed changes

facebook-github-bot closed this in 8614860 Jan 9, 2020

facebook-github-bot added the merged label Jan 9, 2020

facebook-github-bot deleted the gh/ezyang/579/head branch January 13, 2020 15:39

peterjc123 mentioned this pull request Jan 22, 2020

Nightly build failure on Windows pytorch/vision#1756

Closed

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uniformly apply Windows logic in cpp_extensions everywhere #31161

Uniformly apply Windows logic in cpp_extensions everywhere #31161

ezyang commented Dec 12, 2019 •

edited

kostmo commented Dec 12, 2019 •

edited

soumith left a comment

yf225 commented Jan 8, 2020

peterjc123 Jan 8, 2020

ezyang Jan 8, 2020

peterjc123 Jan 8, 2020

peterjc123 Jan 8, 2020

facebook-github-bot commented Jan 9, 2020

Uniformly apply Windows logic in cpp_extensions everywhere #31161

Uniformly apply Windows logic in cpp_extensions everywhere #31161

Conversation

ezyang commented Dec 12, 2019 • edited

kostmo commented Dec 12, 2019 • edited

💊 CircleCI build failures summary and remediations

Detailed failure analysis

❄️ 2 failures recognized as flaky

pytorch_macos_10_13_py3_test (1/2)

pytorch_linux_xenial_cuda9_cudnn7_py3_NO_AVX_NO_AVX2_test (2/2)

1 failure not recognized by patterns:

soumith left a comment

Choose a reason for hiding this comment

yf225 commented Jan 8, 2020

peterjc123 Jan 8, 2020

Choose a reason for hiding this comment

ezyang Jan 8, 2020

Choose a reason for hiding this comment

peterjc123 Jan 8, 2020

Choose a reason for hiding this comment

peterjc123 Jan 8, 2020

Choose a reason for hiding this comment

facebook-github-bot commented Jan 9, 2020

ezyang commented Dec 12, 2019 •

edited

kostmo commented Dec 12, 2019 •

edited