Skip to content
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 numpy v2 breaking changes #618

Merged
merged 6 commits into from
Aug 14, 2024
Merged

Fix numpy v2 breaking changes #618

merged 6 commits into from
Aug 14, 2024

Conversation

fjosw
Copy link
Collaborator

@fjosw fjosw commented Apr 1, 2024

The upcoming numpy v2.0.0 release introduces a few breaking changes for autograd. This PR fixes these issues:

  • msort was removed from the numpy API. I added conditional imports. The corresponding VJPs are now only defined if the numpy version is <2.
  • np.array(value, copy=False) was removed. The same effect can now be achieved via np.asarray(value).
  • The broadcasting rules for np.solve changed.
  • np.linalg.linalg was renamed to np.linalg._linalg.

One thing that I did not adress in this PR is the changing behavior for np.sign with complex arguments. Instead of the sign of the real part, numpy now returns z /|z|.

On my machine all tests pass with the latest release versions (numpy==1.26.4 scipy==1.12.0) and the current release candidates (numpy==2.0.0.rc1 scipy==1.13.0rc1) but for tests/test_systematic.py::test_sign which fails for numpy v2 with

tests/numpy_utils.py:37: in unary_ufunc_check
    check([comp, matc])
autograd/test_util.py:77: in _combo_check
    _check_grads(fun)(*_args, **dict(_kwargs))
autograd/wrap_util.py:20: in nary_f
    return unary_operator(unary_f, x, *nary_op_args, **nary_op_kwargs)
autograd/test_util.py:56: in check_grads
    check_jvp(f, x)
autograd/test_util.py:43: in check_jvp
    check_equivalent(jvp(x_v)[1], jvp_numeric(x_v))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

x = array(0.+0.j), y = np.complex128(-0.2141399934041388+1.070699966299049j)

because of the aforementioned changes to the behavior of the sign function.

@j-towns
Copy link
Collaborator

j-towns commented Apr 2, 2024

Thanks for this @fjosw. I will try to take a look at some point in the next couple of weeks.

@agriyakhetarpal
Copy link
Collaborator

Hi there, @j-towns! I did see from #620 and I am cognizant of the announcement that autograd will not be maintained anymore, however, this PR would be a great service for dependents of autograd and to the community ((8.2k off GitHub's numbers alone), especially with the immensity of the NumPy v2 release. If there is anything that can be done in this regard as a one-off effort/release (v1.6.3) even after the newly-raised status for the project, it would be great to hear from you. June 16, 2024 is the D-Day for the NumPy release.

@j-towns
Copy link
Collaborator

j-towns commented Jun 19, 2024

Hi @agriyakhetarpal, I emailed you with a proposal, have you seen it?

@agriyakhetarpal
Copy link
Collaborator

Hi @j-towns, thanks for sending it out – just responded there. It went to my spam folder, sadly :( I could have responded much earlier otherwise.

@j-towns
Copy link
Collaborator

j-towns commented Jul 6, 2024

@agriyakhetarpal I emailed you again (11 days ago), did you see that? I have also now emailed @fjosw with a similar proposal.

@fjosw
Copy link
Collaborator Author

fjosw commented Jul 7, 2024

Thanks @j-towns, I just sent you a reply.

@agriyakhetarpal
Copy link
Collaborator

Thank you for the ping, @j-towns – just responded!

@fjosw
Copy link
Collaborator Author

fjosw commented Jul 26, 2024

I looked a bit into the changes related to the complex sign function today. It is my understanding that the test fails because the numerical approximation of jvp via finite difference does not work when the sign function is defined as $\mathrm{sgn}(z)=z/|z|$. For this reason I disabled the complex checks for the sign function.

However, I think autograd still does the correct thing when the definition of the sign function changes. I verified explicitly that autograd.holomorphic_grad and jax.grad with holomorphic=True give the same results when differentiating functions containing np.sign (jax also uses $\mathrm{sgn}(z)=z/|z|$).

Copy link
Collaborator

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

I enabled the "Always suggest updating pull request branches" option in the settings and brought in the changes from #624. I think this should be good to go once the tests pass.

@fjosw
Copy link
Collaborator Author

fjosw commented Jul 30, 2024

There seems to be an issue with the windows pypy-3.10 pipeline, unrelated to the code changes. Do we want/need to support pypy runners if they are more error prone @agriyakhetarpal?

@agriyakhetarpal
Copy link
Collaborator

agriyakhetarpal commented Jul 30, 2024

I triggered the test again, @fjosw (let's hope the third time's the charm). I've never worked with PyPy before, so I'm not sure if a test is flaky or if there's a problem with the PyPy set-up in general. I do think we should keep them for this release, however, and evaluate if we wish to drop support later.

@agriyakhetarpal
Copy link
Collaborator

From the error code, it looks like an unrelated error related to the installation rather than with running the tests, and I found something related here: https://stackoverflow.com/a/51817869/14001839. I'll migrate to Hatchling as the build backend after the release, should be doable with not much to do.

@agriyakhetarpal
Copy link
Collaborator

(Or, we can do so before the release and get done with it – and leave it to after the release if it becomes troublesome?)

@fjosw
Copy link
Collaborator Author

fjosw commented Jul 30, 2024

I would not expect any of the autograd code to behave differently with pypy, so I would be happy to drop all the pypy runners and just test with CPython. But if you want to migrate the build backend before a release, that is of course fine with me.

@agriyakhetarpal
Copy link
Collaborator

Fair enough – I don't expect it to be different either, given that the job was passing earlier. I'll put together a PR soonish.

@Alex-Preciado
Copy link

Hi @fjosw, @agriyakhetarpal, @j-towns 👋🏼

Thank you for your work on this PR to add NumPy 2.0 compatibility. Our Quantum Computing library, PennyLane, currently depends on Autograd, and we’re aiming to include NumPy 2.0 support in our upcoming release scheduled for early September. We’re eager to incorporate and test the changes introduced here.

Since this PR seems to be approved, would it be possible to know if merging it within the next few days is feasible? This information would help us determine if we can include NumPy 2.0 support in our release and plan accordingly.

@fjosw
Copy link
Collaborator Author

fjosw commented Aug 13, 2024

Hi @Alex-Preciado, we are currently reworking the build process in #626 and would then merge this PR and prepare a new release with Numpy 2 compatibility. Hard to make promises, but my hope would be to finish this up before the end of the month.

@agriyakhetarpal
Copy link
Collaborator

Thanks for your comment, @Alex-Preciado! As @fjosw mentioned, we'll try our best to get this done as soon as possible. The changes are already approved, though we'll need to coordinate with @j-towns for the release process during the month.

@fjosw fjosw merged commit 84c42fb into HIPS:master Aug 14, 2024
18 checks passed
@fjosw
Copy link
Collaborator Author

fjosw commented Aug 14, 2024

With #626 and this PR merged we should be ready to prepare a new release or is there anything else you would like to get done first @agriyakhetarpal @j-towns ?

@Alex-Preciado
Copy link

Thank you so much for the updates, @fjosw and @agriyakhetarpal !! We’re glad to see this has been merged. We’ll continue to follow the conversation here and look forward to the new release.

@agriyakhetarpal
Copy link
Collaborator

@fjosw, I think a go-through of the settings and the release process would be great! Also, it might be a good time to publish GitHub releases and let the PyPI deployment proceed from there (especially since we are switching to OIDC trusted publishing), rather than triggering the workflow on tags.

@fjosw
Copy link
Collaborator Author

fjosw commented Aug 15, 2024

Do you want to take the lead on that @agriyakhetarpal or do you want me to take care of certain things?

@agriyakhetarpal
Copy link
Collaborator

agriyakhetarpal commented Aug 15, 2024

I could take the lead on that and ask you for help where needed, @fjosw, thanks :) I'm mostly waiting for Jamie to come back from vacation, and then we can get done with the release. We can leave out the rest of the infra changes I have in mind and code cleanup to be done in follow-up releases.

@j-towns
Copy link
Collaborator

j-towns commented Aug 15, 2024

I'm back :). Lmk what I need to do.

@agriyakhetarpal
Copy link
Collaborator

@j-towns, I'm assuming we have secrets for PyPI configured, so we need to create and push a tag, because that's when https://github.com/HIPS/autograd/blob/master/.github/workflows/publish.yml will be running. It wouldn't take a lot of time to set up a trusted environment and the rest of the settings, but I don't have access to those areas of our settings. Also, on second thought, we wouldn't want to hold up the release for others waiting on it either, so maybe we can go ahead with the release now, and I can open PRs to update the rest in the meantime?

@j-towns
Copy link
Collaborator

j-towns commented Aug 15, 2024

I think I already setup the trusted environment in GitHub (I did that on our call) and also the settings on the PyPI end (see email).

@agriyakhetarpal
Copy link
Collaborator

Great! In that case, I'll fix up the release workflow, most likely over the weekend.

@agriyakhetarpal
Copy link
Collaborator

Apologies for the short delay. PR here: #627

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.

4 participants