-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[mutable-arrays] fix bugs with vjp3 exposed by #32470 #32583
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
Conversation
I believe all remaining failures are expected. Co-authored-by: Yash Katariya <[email protected]>
Summary of ChangesHello @mattjj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a series of targeted fixes and improvements primarily focused on enhancing the stability and correctness of JAX's automatic differentiation ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a series of fixes for bugs related to vjp3
and mutable arrays, which were exposed by a recent change. The changes span across automatic differentiation utilities, core interpreters, and state primitives, improving correctness and robustness, especially concerning float0
types and partial evaluation rules. The test modifications also enhance their reliability. My main feedback is on a broad exception handler that could be made more specific to avoid hiding potential future bugs.
try: args[i]._refs._buf._replace_with(o) # type: ignore | ||
except AttributeError: pass # TODO(mattjj): remove float0 |
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.
Using a broad except AttributeError: pass
can be risky as it might silence unexpected errors. While the TODO comment provides context about float0
, it would be safer to make the check more specific if possible. For instance, you could check if the argument is of a type that is known not to have the _refs
attribute, like float0
arrays. If a specific check is not feasible, consider adding a log warning within the except
block to aid in debugging any future, unrelated AttributeError
s that might be suppressed here.
I believe all remaining failures are expected.
Thanks to #32470 for exposing these!