-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: remove redundant(?) Jax.is_tracer_type check in _layout #3013
Conversation
6a9095d
to
ad48d89
Compare
Codecov ReportAttention:
Additional details and impacted files
|
Should I install jax in the workflows or are we not supposed to use non-dependency libraries in the tests? (or maybe |
That looks fine, the reproducer should have returned a scalar from the function being differentiated. When doing that by using e.g. |
@Saransh-cpp |
@Saransh-cpp this has my approval if you want to "see what happens". My feeling is that our JAX user base is small enough that we can move fast and break things (I'm sure Jim agrees). |
399b386
to
16f947c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know of anything wrong with this. We can just remove it and "see what happens" because it only affects users of the JAX backend. In general, we can't move fast and break things with this library, but only for a limited context like this backend.
This guard was added by @agoose77 in #2389, and maybe he remembers why it was added.
I approve the PR to be merged. If, after merging, we find out that it was not a good idea, you know how to reinstate it.
Noted, thanks! |
Fixes #2556 #2637
I am still not completely sure if removing the check is safe, but everything works for me locally, including the non-jax part of awkward.
#2637 now exits with a new error which is on the user side and not on the awkward side -
cc: @alexander-held