KEMBAR78
Extend torch function support to ALL arguments, not just scalar type (but not insides of list) by ezyang · Pull Request #145089 · pytorch/pytorch · GitHub
Skip to content

Conversation

@ezyang
Copy link
Contributor

@ezyang ezyang commented Jan 17, 2025

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Jan 17, 2025

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit ac58da9 with merge base 2507ae6 (image):

UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:

  • pull / linux-jammy-py3_9-clang9-xla / test (xla, 1, 1, linux.12xlarge, unstable) (gh) (#158876)
    /var/lib/jenkins/workspace/xla/torch_xla/csrc/runtime/BUILD:476:14: Compiling torch_xla/csrc/runtime/xla_util_test.cpp failed: (Exit 1): gcc failed: error executing CppCompile command (from target //torch_xla/csrc/runtime:xla_util_test) /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -g0 -O2 '-D_FORTIFY_SOURCE=1' -DNDEBUG -ffunction-sections ... (remaining 229 arguments skipped)

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

ezyang added a commit that referenced this pull request Jan 17, 2025
… type (but not insides of list)

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

ghstack-source-id: 389c821
Pull Request resolved: #145089
@ezyang ezyang requested a review from vmoens January 21, 2025 14:36
@ezyang
Copy link
Contributor Author

ezyang commented Jan 24, 2025

The specific problem relates to torch function redispatch. Consider the argument overload list for Tensor.set_:

  static PythonArgParser parser(
      {
          "set_()",
          "set_(Storage source)",
          "set_(Storage source, SymInt storage_offset, SymIntArrayRef size, SymIntArrayRef stride=None)",
          "set_(Tensor source)",
          "set_(Tensor source, SymInt storage_offset, SymIntArrayRef size, SymIntArrayRef stride=None)",
      },
      /*traceable=*/false);

This says we attempt to resolve the Storage overload before the Tensor overload. When a Tensor has a __torch_function__ on it, the current implementation of the patch causes it to match the Storage (because an object with torch function always matches). Now the problem is when we redispatch with torch function disabled, it STILL matches and we end up attempting the Storage overload.

So maybe we need some special case saying that if something is a Tensor, it doesn't match for non-Tensor arguments? Hmm...

[ghstack-poisoned]
@pytorch-bot pytorch-bot bot added the release notes: fx release notes category label Jan 24, 2025
[ghstack-poisoned]
ezyang added a commit that referenced this pull request Jan 24, 2025
… type (but not insides of list)

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

ghstack-source-id: dcef210
Pull Request resolved: #145089
@ezyang ezyang changed the title [POC] Extend torch function support to ALL arguments, not just scalar type (but not insides of list) Extend torch function support to ALL arguments, not just scalar type (but not insides of list) Jan 25, 2025
@ezyang
Copy link
Contributor Author

ezyang commented Jan 25, 2025

@vmoens @albanD I don't really know if I want to land this before I go on leave, it might have unintended side effects. Also, it doesn't work with entries in lists, which probably should be done before this is considered feature complete.

@ezyang ezyang changed the title Extend torch function support to ALL arguments, not just scalar type (but not insides of list) [POC] Extend torch function support to ALL arguments, not just scalar type (but not insides of list) Jan 25, 2025
@vmoens
Copy link
Contributor

vmoens commented Jan 25, 2025

Sure I'm in no hurry for this to land, we can talk about the implications once you're back!

@ezyang ezyang changed the title [POC] Extend torch function support to ALL arguments, not just scalar type (but not insides of list) Extend torch function support to ALL arguments, not just scalar type (but not insides of list) Aug 7, 2025
@ezyang
Copy link
Contributor Author

ezyang commented Aug 7, 2025

We ballin' this

[ghstack-poisoned]
ezyang added a commit that referenced this pull request Aug 7, 2025
… type (but not insides of list)

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

ghstack-source-id: de8ef4a
Pull Request resolved: #145089
@ezyang ezyang added topic: new features topic category module: python frontend For issues relating to PyTorch's Python frontend and removed no-stale labels Aug 7, 2025
@ezyang ezyang requested review from zou3519 and removed request for vmoens August 7, 2025 14:55
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yolo I guess? Not sure if we want more extensive testing for this..

// that internally
if (check_has_torch_function(obj, /*ignore_mode*/ true) &&
!THPVariable_Check(obj)) {
// tensor subclasses and unrelated objects with __torch_function__
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs updating as only non-subclass are caught here actually.

[ghstack-poisoned]
@ezyang
Copy link
Contributor Author

ezyang commented Aug 7, 2025

@pytorchbot merge

ezyang added a commit that referenced this pull request Aug 7, 2025
… type (but not insides of list)

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

ghstack-source-id: 3046eb6
Pull Request resolved: #145089
@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Aug 7, 2025
@pytorchmergebot
Copy link
Collaborator

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

hinriksnaer pushed a commit to hinriksnaer/pytorch that referenced this pull request Aug 8, 2025
…(but not insides of list) (pytorch#145089)

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

Pull Request resolved: pytorch#145089
Approved by: https://github.com/albanD, https://github.com/zou3519
@github-actions github-actions bot deleted the gh/ezyang/3068/head branch September 7, 2025 02:14
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
…(but not insides of list) (pytorch#145089)

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

Pull Request resolved: pytorch#145089
Approved by: https://github.com/albanD, https://github.com/zou3519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged module: python frontend For issues relating to PyTorch's Python frontend release notes: fx release notes category topic: new features topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants