Hide torch_python symbols #142214

cyyever · 2024-12-06T03:35:27Z

Change symbols in torch_python to invisible by default on platforms other than Apple.

cc @albanD

pytorch-bot · 2024-12-06T03:35:31Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ You can merge normally! (3 Unrelated Failures)

As of commit 20fae6f with merge base e0bdae7 ():

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

periodic / linux-focal-cuda11.8-py3.10-gcc9-debug / test (default, 3, 5, lf.linux.4xlarge.nvidia.gpu, oncall:debug-build) (gh) (trunk failure)
test_nestedtensor.py::TestNestedTensorSubclassCUDA::test_linear_nt_dim_3_cuda
periodic / linux-focal-cuda11.8-py3.9-gcc9 / test (multigpu, 1, 1, lf.linux.g5.12xlarge.nvidia.gpu, oncall:distributed) (gh) (trunk failure)
'Test'
periodic / linux-focal-cuda12.1-py3-gcc9-slow-gradcheck / test (default, 2, 8, linux.g5.4xlarge.nvidia.gpu, module:slowgradcheck) (gh) (trunk failure)
test_nestedtensor.py::TestNestedTensorOpInfoCUDA::test_backward_prod_cuda_float32

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

netlify · 2024-12-06T03:38:42Z

✅ Deploy Preview for chimerical-cranachan-793287 ready!

Name	Link
🔨 Latest commit	525351ef2565dce6bfbd6fca88d309dee3a7e770
🔍 Latest deploy log	https://app.netlify.com/sites/chimerical-cranachan-793287/deploys/675324397306200008c9f2ab
😎 Deploy Preview	https://deploy-preview-142214--chimerical-cranachan-793287.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Skylion007 · 2024-12-09T14:51:59Z

What's the point of doing this and why weren't we hiding symbols in it before? Does this guard against any invalidly compiled PyTorch extensions silently failing?

cyyever · 2024-12-09T14:56:30Z

What's the point of doing this and why weren't we hiding symbols in it before? Does this guard against any invalidly compiled PyTorch extensions silently failing?

The torch_python exposed all symbols before. Recently it was wrapped into torch_compile_options with default visibility. This PR tries to mark public symbols explicitly. We should have a clear definition of public python API. Some extensions using private symbols will see linker failures.

albanD · 2024-12-10T20:11:37Z

I'm not sure how to test this :/
Can you share the PR that makes that visibility change?

cyyever · 2024-12-11T00:37:31Z

It is #136743

ezyang

This is kind of risky but I guess we can give it a try.

janeyx99 · 2024-12-11T15:54:11Z

torch/CMakeLists.txt

-if(NOT WIN32)
-  target_compile_options(torch_python PRIVATE
-      $<$<COMPILE_LANGUAGE:CXX>: -fvisibility=default>)
-endif()


@cyyever how come this change undoes the previous change #136743?

And is it intentional that line 313 is commented out but not deleted?

torch_compile_options uses private visibility for torch targets. Line 313 is from merge conflicting that is now solved.

janeyx99 · 2024-12-11T15:54:58Z

torch/csrc/Layout.h

 };

-extern PyTypeObject THPLayoutType;
+TORCH_PYTHON_API extern PyTypeObject THPLayoutType;


How did you decide which 3 APIs to start labeling with TORCH_PYTHON_API?

Will this change break extensions that use other APIs?

I built it locally and fixed all import errors. The 3 APIs were found during that time.

albanD · 2024-12-11T16:01:00Z

Curious what is the plan to test this to make sure we didn't miss others? Did you try on a set of third party extensions?

cyyever · 2024-12-12T01:02:01Z

Curious what is the plan to test this to make sure we didn't miss others? Did you try on a set of third party extensions?

Tried it on hugging face libraries and torchvison, it works fine.

cyyever · 2024-12-12T02:27:22Z

@pytorchmergebot merge

pytorchmergebot · 2024-12-12T02:29:04Z

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

pytorchmergebot · 2024-12-12T02:50:34Z

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / macos-py3-arm64 / test (default, 1, 3, macos-m1-stable)

Details for Dev Infra team

Raised by workflow job

cyyever · 2024-12-16T00:52:07Z

@pytorchmergebot merge -i

pytorchmergebot · 2024-12-16T00:53:48Z

Merge started

Your change will be merged while ignoring the following 3 checks: periodic / linux-focal-cuda12.1-py3-gcc9-slow-gradcheck / test (default, 2, 8, linux.g5.4xlarge.nvidia.gpu, module:slowgradcheck), periodic / linux-focal-cuda11.8-py3.9-gcc9 / test (multigpu, 1, 1, lf.linux.g5.12xlarge.nvidia.gpu, oncall:distributed), periodic / linux-focal-cuda11.8-py3.10-gcc9-debug / test (default, 3, 5, lf.linux.4xlarge.nvidia.gpu, oncall:debug-build)

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

albanD · 2024-12-18T11:10:46Z

I actually would like to argue to revert this one again.
It broke a bunch of things including some internal use case that had to be fixed in #143380 (comment) and it is also breaking the WIP extension at https://github.com/pytorch/pytorch/tree/main/test/cpp_extensions/open_registration_extension (working on enabling it in CI but failing now).

I am afraid there are a lot more of these APIs that would need to be marked and as you mentioned in the other PR, we don't have a good way right now to make sure we caught all of them.
Also @atalman can you confirm this did NOT make it into the release. If it did, we should definitely remove it!

malfet · 2024-12-18T15:45:14Z

torch/CMakeLists.txt

 add_library(torch_python SHARED ${TORCH_PYTHON_SRCS})
 torch_compile_options(torch_python)  # see cmake/public/utils.cmake
-if(NOT WIN32)
+if(APPLE)


Why this check here? Imo symbol visibility on Linux and Mac should be identical, shouldn't it?

Apple has some hard to find linking errors that lead to test failures

malfet · 2024-12-18T15:47:08Z

Wow, I'm surprised we didn't do it before. I'm very supportive of the change, but also a bit scared of the things it could break. (Though, why keep default visibility on Mac?)

Can you confirm this did NOT make it into the release. If it did, we should definitely remove it!
It's not part of the release, according to

git log origin/release/2.6 -- torch/CMakeLists.txt

and https://github.com/pytorch/pytorch/commits/release/2.6/torch/CMakeLists.txt

suo · 2024-12-19T05:47:29Z

I think this change makes sense directionally, but making a public symbol private is a breaking change so we should be careful about it. I think it would be good to revert and come up with a plan for how to roll it out.

cyyever · 2024-12-19T06:14:27Z

Another way is to gradually discover those symbols and mark them public.

albanD · 2024-12-19T09:40:10Z

The problem with this approach @cyyever is that users based on the release will have a 3months delay where they're unable to use PyTorch until the next release comes out. Which is really not acceptable.

cyyever · 2024-12-19T10:21:09Z

@albanD Fully understand, seems that it should be reverted.

atalman · 2024-12-19T14:24:05Z

@albanD @malfet I confirm this is not part of the release 2.6:
https://github.com/pytorch/pytorch/blob/release/2.6/torch/CMakeLists.txt#L313

Fixes #148208. There are solutions for exposing symbols implicitly from inline functions (i.e., inline function A calls non-inline function B in foo.h. Code includes foo.h has to see the symbol B in DLL). Solution 1: tag the entire struct where the inline functions are defined as member functions with TORCH_PYTHON_API --- this PR does this for python_arg_parser.h. An alternative solution exists but will slow down dispatching a lot --- drop inline keyword and move implementation to .cc file. Solution 2: tag individual functions with TORCH_PYTHON_API. This PR does this for python_tensor.h. Related discussion about hiding torch_python symbols: #142214 Pull Request resolved: #148213 Approved by: https://github.com/malfet

pytorch-bot bot added the topic: not user facing topic category label Dec 6, 2024

cyyever marked this pull request as draft December 6, 2024 03:35

pytorchbot added the open source label Dec 6, 2024

cyyever force-pushed the torch_python_abi branch 2 times, most recently from 8b584ca to 525351e Compare December 6, 2024 16:20

cyyever changed the title ~~Hide torch_python_symbols~~ Hide torch_python symbols Dec 7, 2024

cyyever marked this pull request as ready for review December 7, 2024 05:12

cyyever added the ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR label Dec 8, 2024

cyyever force-pushed the torch_python_abi branch from 525351e to acc57b4 Compare December 8, 2024 04:31

cyyever requested a review from ezyang December 8, 2024 13:22

janeyx99 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Dec 9, 2024

ezyang requested review from albanD and malfet December 10, 2024 04:40

ezyang requested a review from janeyx99 December 11, 2024 15:40

ezyang approved these changes Dec 11, 2024

View reviewed changes

janeyx99 reviewed Dec 11, 2024

View reviewed changes

cyyever added the ciflow/s390 s390x-related CI jobs label Dec 12, 2024

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Dec 12, 2024

pytorchmergebot added the merging label Dec 12, 2024

Export only on Apple platforms

20fae6f

cyyever force-pushed the torch_python_abi branch from c8f3628 to 20fae6f Compare December 15, 2024 13:42

cyyever marked this pull request as ready for review December 16, 2024 00:48

cyyever requested review from mikaylagawarecki and soulitzer as code owners December 16, 2024 00:48

pytorchmergebot added the merging label Dec 16, 2024

pytorchmergebot closed this in af8789c Dec 16, 2024

pytorchmergebot removed the merging label Dec 16, 2024

malfet reviewed Dec 18, 2024

View reviewed changes

Zhenbin-8 mentioned this pull request Dec 23, 2024

[1/N] OpenReg: Replace open_registration_extension.cpp with openreg #141815

Closed

malfet mentioned this pull request Jan 7, 2025

Cannot get torch_python symbol #144302

Closed

wschin mentioned this pull request Feb 28, 2025

Expose functions used in custom backend in torch_python dll #148213

Closed

malfet added topic: bc breaking topic category module: python frontend For issues relating to PyTorch's Python frontend and removed topic: not user facing topic category labels Apr 9, 2025

janeyx99 added the release notes: cpp release notes category label Apr 9, 2025

cyyever deleted the torch_python_abi branch May 14, 2025 01:36

Skylion007 mentioned this pull request May 23, 2025

tensor_to_numpy symbol not exported in libtorch_python.so in pytorch 2.7 #154105

Closed

Hide torch_python symbols #142214

Hide torch_python symbols #142214

Uh oh!

Conversation

cyyever commented Dec 6, 2024 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ You can merge normally! (3 Unrelated Failures)

Uh oh!

netlify bot commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for chimerical-cranachan-793287 ready!

Uh oh!

Skylion007 commented Dec 9, 2024

Uh oh!

cyyever commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albanD commented Dec 10, 2024

Uh oh!

cyyever commented Dec 11, 2024

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

janeyx99 Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

cyyever Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janeyx99 Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

cyyever Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

albanD commented Dec 11, 2024

Uh oh!

cyyever commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyyever commented Dec 12, 2024

Uh oh!

pytorchmergebot commented Dec 12, 2024

Merge started

Uh oh!

pytorchmergebot commented Dec 12, 2024

Merge failed

Uh oh!

cyyever commented Dec 16, 2024

Uh oh!

pytorchmergebot commented Dec 16, 2024

Merge started

Uh oh!

albanD commented Dec 18, 2024

Uh oh!

malfet Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

cyyever Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

malfet commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suo commented Dec 19, 2024

Uh oh!

cyyever commented Dec 19, 2024

Uh oh!

albanD commented Dec 19, 2024

Uh oh!

cyyever commented Dec 19, 2024

Uh oh!

atalman commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cyyever commented Dec 6, 2024 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Dec 6, 2024 •

edited

Loading

netlify bot commented Dec 6, 2024 •

edited

Loading

cyyever commented Dec 9, 2024 •

edited

Loading

cyyever Dec 12, 2024 •

edited

Loading

cyyever commented Dec 12, 2024 •

edited

Loading

malfet commented Dec 18, 2024 •

edited

Loading

atalman commented Dec 19, 2024 •

edited

Loading