Skip to content

Automatically add out kwarg & member & inplace_member#19

Merged
samuela merged 29 commits into
samuela:mainfrom
mavenlin:out_arg_and_member
Aug 21, 2025
Merged

Automatically add out kwarg & member & inplace_member#19
samuela merged 29 commits into
samuela:mainfrom
mavenlin:out_arg_and_member

Conversation

@mavenlin

@mavenlin mavenlin commented Aug 8, 2025

Copy link
Copy Markdown
Contributor

As per our discussion in #18,

  • For functions that support out=None kwarg, we use a preset template in the decorator.
  • Automatically set the member functions when they exist to avoid manually add them at two places. When we implement torch.add, torch.Tensor.add and torch.Tensor.add_ are automatically added. While it works for most of the functions, for some of the functions this can be turned off, e.g. torch.permute & torch.Tensor.permute has different signatures.

@mavenlin
mavenlin force-pushed the out_arg_and_member branch from cf98a31 to 0cc24ea Compare August 8, 2025 13:13

@samuela samuela left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for putting this together @mavenlin ! Let's also write some automated tests for all these out=tensor things and Tensor.foo_ methods

Comment thread torch2jax/__init__.py Outdated
Comment thread torch2jax/__init__.py Outdated
@mavenlin
mavenlin force-pushed the out_arg_and_member branch 2 times, most recently from 3f1380a to 0551865 Compare August 10, 2025 06:46
@mavenlin

Copy link
Copy Markdown
Contributor Author

To enable automatic tests of out, foo & foo_, I did some refactoring on the test code.
Summarized as follows

  • To automatically test member functions, the tests are required to directly take the original torch.foo instead of lambda x: torch.foo(x, a=1, b=2). Therefore, the kwargs {a=1, b=2} needs to be passed instead of binding via lambda functions.
  • Use tree_map to handle the output of t2j_function, so that functions like torch.unbind can be tested as it is, without needing to wrap with a lambda.
  • Testing with test_t2j_function(lambda ... will still go through forward & backward tests like the original code, but will not be tested for out and member functions.

It is not in the best way I would like it though, let me know whatever suggestions you have.

Comment thread torch2jax/__init__.py Outdated
Comment thread torch2jax/__init__.py
@mavenlin
mavenlin force-pushed the out_arg_and_member branch 2 times, most recently from 881c03e to 0f7c371 Compare August 14, 2025 10:37
@mavenlin

Copy link
Copy Markdown
Contributor Author

The CI error seems to be from nix version before the checkout step, I'm not very familiar with that, @samuela could you help take a look?

@samuela

samuela commented Aug 15, 2025

Copy link
Copy Markdown
Owner

yes, sorry about the ci. it should be fixed after b958825

@mavenlin
mavenlin force-pushed the out_arg_and_member branch from 0f7c371 to f50cb05 Compare August 15, 2025 04:53
@mavenlin

Copy link
Copy Markdown
Contributor Author

Great, tests are passing after the ci fix. Let me know if you have further comments on this one.

@samuela

samuela commented Aug 15, 2025

Copy link
Copy Markdown
Owner

looks like there are some merge conflicts. i'm also gonna have to meditate a bit on how exactly i want to design this. what u have is great, but i'm concerned we may eventually blow up the complexity of implements

@mavenlin

mavenlin commented Aug 16, 2025

Copy link
Copy Markdown
Contributor Author

looks like there are some merge conflicts. i'm also gonna have to meditate a bit on how exactly i want to design this. what u have is great, but i'm concerned we may eventually blow up the complexity of implements

The awkward part is that whether out_kwarg is added interferes with Torchishify_output. Adding member to torch.Tensor also requires the overridden torch function's name.

Comment thread tests/utils.py Outdated
Comment thread tests/utils.py Outdated
@mavenlin
mavenlin force-pushed the out_arg_and_member branch from f50cb05 to a898b6a Compare August 17, 2025 14:18
Comment thread tests/utils.py Outdated
Comment thread tests/utils.py Outdated
Comment thread tests/utils.py Outdated
Comment thread tests/utils.py Outdated
Comment thread tests/utils.py Outdated
Comment thread tests/utils.py Outdated
Comment thread tests/utils.py Outdated
Comment thread tests/utils.py Outdated
Comment thread tests/utils.py Outdated
@mavenlin

Copy link
Copy Markdown
Contributor Author

2025-08-19

  • Explicit control what to test for each function
  • Test gradient for all cases by reducing output to scalar.
  • Fix gradient of batch_norm

@samuela
samuela merged commit 6dfa95e into samuela:main Aug 21, 2025
1 check passed
@samuela

samuela commented Aug 21, 2025

Copy link
Copy Markdown
Owner

Thanks @mavenlin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants