Skip to content

with torch.no_grad#21

Merged
samuela merged 5 commits into
samuela:mainfrom
mavenlin:no_grad
Aug 15, 2025
Merged

with torch.no_grad#21
samuela merged 5 commits into
samuela:mainfrom
mavenlin:no_grad

Conversation

@mavenlin

Copy link
Copy Markdown
Contributor

When using torch.no_grad, an error is raised complaining about torch._C.set_grad_enabled is not implemented for Torchish.

This PR added a global variable to track whether gradient is enabled. Any new Torchish element created under the no_grad context will have its .value wrapped with lax.stop_gradient.

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

Comment thread torch2jax/__init__.py Outdated
_RNG_POOPER_STACK.pop()


_GRAD_ENABLED = True

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.

let's add a comment here documenting why this exists and what parts of the api it interacts with

@mavenlin mavenlin Aug 14, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Replaced with forwarding torch._C._set_grad_enabled to its original implementation, which is better than this global variable.

It is somehow necessary, otherwise torch will complain about no __torch_function__ handler defined for torch._C._set_grad_enabled.

Comment thread torch2jax/__init__.py Outdated
@mavenlin

Copy link
Copy Markdown
Contributor Author

i'm hesitant to add a global variable but this may indeed be the simplest route. at some point we ought to also consider supporting/testing https://docs.pytorch.org/docs/stable/generated/torch.enable_grad.html, https://docs.pytorch.org/docs/stable/generated/torch.autograd.grad_mode.set_grad_enabled.html, and https://docs.pytorch.org/docs/stable/generated/torch.is_grad_enabled.html

These are added

@mavenlin

mavenlin commented Aug 14, 2025

Copy link
Copy Markdown
Contributor Author

BTW, I've changed value to be a property and added a setter, in this way, the inplace operations will have correct gradient.

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

nice! reusing torch's internal thing is still global state but hey at least it's not our global state :)

Comment thread tests/test_core.py Outdated
Comment thread torch2jax/__init__.py Outdated
@samuela

samuela commented Aug 15, 2025

Copy link
Copy Markdown
Owner

ci should be fixed after b958825

@mavenlin

Copy link
Copy Markdown
Contributor Author

ci should be fixed after b958825

Great, thanks!

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

it's worth mentioning that

with torch.no_grad():
  jax.grad(t2j(torch.sin))(x)

will be None or zero. it's debatable what should really happen here. i'm fine with merging as is and we can revisit later if necessary

@samuela
samuela merged commit ec3da3a into samuela:main Aug 15, 2025
1 check passed
@mavenlin
mavenlin deleted the no_grad branch August 23, 2025 14:08
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