Expose functions used in custom backend in torch_python dll #148213

wschin · 2025-02-28T21:26:00Z

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

pytorch-bot · 2025-02-28T21:26:04Z

🔗 Helpful Links

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

📄 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.

✅ No Failures

As of commit e0370b0 with merge base 4216478 ():
💚 Looks good so far! There are no failures yet. 💚

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

wschin · 2025-02-28T21:38:02Z

@pytorchbot label "topic: not user facing"

shink · 2025-03-03T11:50:35Z

torch/csrc/tensor/python_tensor.h

+TORCH_PYTHON_API c10::DispatchKey get_default_dispatch_key();
 TORCH_PYTHON_API at::Device get_default_device();

 // Gets the ScalarType for the default tensor type.
-at::ScalarType get_default_scalar_type();
+TORCH_PYTHON_API at::ScalarType get_default_scalar_type();


Maybe we should test if this change affects torch_npu cc: @FFFrog

I guess it will just bring some symbols back. Those symbols used to exist but got removed ~3 months ago.

No effect, but adding TORCH_PYTHON_API for private functions is a bit weird to me :/

You're right. So those are moved to public as discussed in #148208.

malfet

IMO it's wrong to say that private class method should be accessible from the extensions. (The rest of the changes are fine)

malfet · 2025-03-03T17:56:09Z

torch/csrc/utils/python_arg_parser.h

+  TORCH_PYTHON_API at::Tensor tensor_slow(int i);
+  TORCH_PYTHON_API at::Scalar scalar_slow(int i);
+  TORCH_PYTHON_API at::Scalar scalar_slow(PyObject* arg);


This feels wrong: we should not make private functions part of the API...

See my PR message on the top. Those are called in (performance critical) inline functions in .h file.

IMO two potential solutions: Add TORCH_PYTHON_API to the entire class, which will make all its methods symbols visible or make those methods public, rather than private to the class implementation

Let me try the 2nd solution, which looks more aligned with the goal of reducing API surace.

albanD · 2025-03-03T19:54:29Z

torch/csrc/utils/python_arg_parser.h

  inline bool isNone(int i);
  inline std::optional<c10::DispatchKeySet> toDispatchKeySetOptional(int i);

- private:


These are not part of the functions that used to be public but moved to private 3 months ago? Why are you adding them here?

Do you now who moved them to private?

The "moved to private" I mention here is the PR from 3 months ago hiding the symbols. Not moving these to be on this class "private:" tag.

I am not sure what do you mean by public. Do you mean public C++ member function or symbols available in DLL? Our custom backend links to these functions when building with PyTorch. Those functions are accessible in PyTorch DLL ~ 1 year ago, so they were exposed in DLL before.

Q: Why are those APIs are tagged with TORCH_PYTHON_API?
A: These private functions are called inline functions in .h file. Whoever includes that .h file and indirectly calls these private functions will need these symbols when linking to torch_python DLL.

Q: Why is C++ private keyword removed?
A: It's a consensus in #148208. We can change tensor_slow back to private C++ member function, but you will see

... private: TORCH_PYTHON_API ... tensor_slow(...)

which is strange because private function becomes an API.

@albanD, @malfet, gentle ping. Please let me know if I missed anything.

Hmm, just curious, would TORCH_PYTHON_API the entire class will work? (That would transitively apply to all methods?)

@malfet, I can try but that's not the conclusion made by other meta folks in #148208. I will update if it works soon.

Tagging TORCH_PYTHON_API at structure level works for my case.

Let's land it this way then

Fix indent

malfet

LGTM, thank you for the updates

malfet · 2025-03-07T02:32:51Z

@pytorchbot merge -f "Everything looks green"

pytorchmergebot · 2025-03-07T02:34:21Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

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

Expose functions used in custom backend in torch_python dll

a27f976

pytorchbot added the open source label Feb 28, 2025

Use right macro

b463f81

wschin force-pushed the expose-some-symbols branch from 7f63068 to b463f81 Compare February 28, 2025 21:37

pytorch-bot bot added the topic: not user facing topic category label Feb 28, 2025

wschin requested review from albanD and malfet February 28, 2025 22:18

wschin added the topic: bug fixes topic category label Mar 2, 2025

shink reviewed Mar 3, 2025

View reviewed changes

malfet requested changes Mar 3, 2025

View reviewed changes

Address comment

94e146c

wschin mentioned this pull request Mar 3, 2025

Regression: Missing Symbols in PyTorch DLL (torch_python) #148208

Closed

janeyx99 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Mar 3, 2025

albanD reviewed Mar 3, 2025

View reviewed changes

Follow latest suggestion

e0370b0

Fix indent

wschin force-pushed the expose-some-symbols branch from e8faba8 to e0370b0 Compare March 5, 2025 23:59

malfet approved these changes Mar 7, 2025

View reviewed changes

pytorchmergebot added the merging label Mar 7, 2025

pytorchmergebot closed this in 9c9b05b Mar 7, 2025

pytorchmergebot added Merged and removed merging labels Mar 7, 2025

Expose functions used in custom backend in torch_python dll #148213

Expose functions used in custom backend in torch_python dll #148213

Uh oh!

Conversation

wschin commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Feb 28, 2025 • 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/148213

✅ No Failures

Uh oh!

wschin commented Feb 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malfet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin Mar 3, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin Mar 5, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malfet left a comment

Choose a reason for hiding this comment

Uh oh!

malfet commented Mar 7, 2025

Uh oh!

pytorchmergebot commented Mar 7, 2025

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

wschin commented Feb 28, 2025 •

edited

Loading

pytorch-bot bot commented Feb 28, 2025 •

edited

Loading

wschin Mar 3, 2025 •

edited

Loading

wschin Mar 5, 2025 •

edited

Loading