flax icon indicating copy to clipboard operation
flax copied to clipboard

Improve error message for when nnx.Modules use jax or numpy arrays as leaf values

Open RaghuSpaceRajan opened this issue 1 year ago • 9 comments

What does this PR do?

Fixes #4480

Checklist

  • [✔] This PR fixes a minor issue (e.g.: typo or small bug) or improves the docs (you can dismiss the other checks if that's the case).

RaghuSpaceRajan avatar Jan 20 '25 15:01 RaghuSpaceRajan

@RaghuSpaceRajan updated main today to fix the broken tests due to JAX's recent update. Can you please rebase?

cgarciae avatar Jan 23 '25 17:01 cgarciae

@cgarciae Done.

RaghuSpaceRajan avatar Jan 24 '25 11:01 RaghuSpaceRajan

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.

RaghuSpaceRajan avatar Jan 24 '25 11:01 RaghuSpaceRajan

Hey @RaghuSpaceRajan , the diff looks good here on github.

cgarciae avatar Feb 10 '25 16:02 cgarciae

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:

  1. The join in the "path is not None" else part of the if statement here

Old code:

        path_str = '/'.join(map(str, (*path, key)))

New code:

          path_str = '/'.join(map(str, path))

Why is the key not needed anymore and why does path not have * anymore?

  1. I hope error message in the part of the code that is in the new "path is None" else part of the if statement here makes sense even with path being None.

Are the changes good and should we merge now?

RaghuSpaceRajan avatar Feb 17 '25 19:02 RaghuSpaceRajan

Hi @RaghuSpaceRajan , the reason is that path is constructed a bit differently and the key is appended early.

Thanks for updating the PR, I'll run the tests.

cgarciae avatar Feb 17 '25 19:02 cgarciae

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?

RaghuSpaceRajan avatar Feb 17 '25 21:02 RaghuSpaceRajan

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?

Sorry - we cannot squash it during merge because merging will be done by an automated system. Can you please squash them? Then I can proceed with merging.

IvyZX avatar Feb 26 '25 22:02 IvyZX

Sorry, I tried to rebase it locally and squash the last 8 commits (as it shows my branch is 8 commits ahead on the GitHub UI), but locally it seems to show 26 commits since my first commit to the branch. I am pretty confused now. Would it be possible to squash and merge this one manually? Otherwise I'll create a new pull request with all changes made in a single commit manually. What do you think?

RaghuSpaceRajan avatar Feb 27 '25 11:02 RaghuSpaceRajan