-
Notifications
You must be signed in to change notification settings - Fork 670
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
Improve error message for when nnx.Modules use jax or numpy arrays as leaf values #4492
base: main
Are you sure you want to change the base?
Conversation
flax/nnx/graph.py
Outdated
@@ -446,7 +446,8 @@ def _graph_flatten( | |||
if isinstance(value, (jax.Array, np.ndarray)): | |||
path_str = '/'.join(map(str, (*path, key))) | |||
raise ValueError( | |||
f'Arrays leaves are not supported, at {path_str!r}: {value}' | |||
f'The variable at the path {path_str!r} with value = {value} is of type:' \ | |||
f'{type(value)}. Leaf values of this type are not supported for nnx.Modules.' |
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.
f'{type(value)}. Leaf values of this type are not supported for nnx.Modules.' | |
f'{type(value)}. Leaf values of this type are not supported for nnx.Modules or nnx.Object in general.' |
@RaghuSpaceRajan updated |
…does nothing (it used to control a checkpointing behavior that has since been optimized away). PiperOrigin-RevId: 718571228
PiperOrigin-RevId: 718899289
@cgarciae Done. |
Actually, for some reason, it shows me as author for some of the rebased commits. Not sure if that's normal, it's the first time I have rebased across forks. Let me know if should just redo the thing. |
Hey @RaghuSpaceRajan , the diff looks good here on github. |
Hi @cgarciae , sorry I was on vacation last week. Some conflict seems to have appeared in the meantime. I added your suggestion and tried to resolve the conflict but I am unsure about two things:
Old code:
New code:
Why is the
Are the changes good and should we merge now? |
Hi @RaghuSpaceRajan , the reason is that Thanks for updating the PR, I'll run the tests. |
Thanks! I guess the only test failing is having more than 5 commits in the merge. I think you should be able to squash and merge and then it should not be a problem, right? |
What does this PR do?
Fixes #4480
Checklist
checks if that's the case).