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

Cleanup CI and Support NumPy 2.0 #148

Merged
merged 22 commits into from
Aug 2, 2024
Merged

Cleanup CI and Support NumPy 2.0 #148

merged 22 commits into from
Aug 2, 2024

Conversation

imciner2
Copy link
Member

Cleanup the CI in the OSQP python repo to more closely match the CI in QDLDL, and work on enabling the NumPy 2.0 support.

…2.4 on linux (latest available from nvidia rhel7 repo)

cuda 12.4 on linux
@vineetbansal
Copy link
Contributor

vineetbansal commented Jul 28, 2024

@imciner2, @AmitSolomonPrinceton - I've done a few things on this branch to make the CI pass:

  1. Since the api for osqp_adjoint_derivative_compute has changed (it only takes a y now instead of y_low and y_high), the wrapper definitions have changed accordingly, as well as the tests. osqp-python is thus pinned to osqp ff3716 (current as of this writing).

    One test that I could not fix after the wrapper change was test_dl_dq_nonzero_dy (original implementation below), so I've disabled it with a leading underscore. One of you who knows whats going on can fix this easily I imagine, and re-enable this test:

 def test_dl_dq_nonzero_dy(self, verbose=False):
        n, m = 6, 3

        prob = self.get_prob(n=n, m=m, P_scale=1.0, A_scale=1.0)
        P, q, A, l, u, true_x, true_yl, true_yu = prob
        # u = l
        # l[20:40] = -osqp.constant('OSQP_INFTY', algebra='builtin')
        num_eq = 2
        u[:num_eq] = l[:num_eq]

        def grad(q, mode):
            _, dq, _, _, _ = self.get_grads(P, q, A, l, u, true_x, true_yl, true_yu, mode=mode)
            return dq

        def f(q):
            m = osqp.OSQP(algebra='builtin')
            m.setup(
                P,
                q,
                A,
                l,
                u,
                eps_abs=eps_abs,
                eps_rel=eps_rel,
                max_iter=max_iter,
                verbose=False,
            )
            res = m.solve()
            if res.info.status != 'solved':
                raise ValueError('Problem not solved!')
            x_hat = res.x
            y_hat = res.y
            yu_hat = np.maximum(y_hat, 0)
            yl_hat = -np.minimum(y_hat, 0)

            # return 0.5 * np.sum(np.square(x_hat - true_x)) + np.sum(yl_hat) + np.sum(yu_hat)
            return 0.5 * (
                np.sum(np.square(x_hat - true_x))
                + np.sum(np.square(yl_hat - true_yl))
                + np.sum(np.square(yu_hat - true_yu))
            )

        dq_qdldl = grad(q, 'qdldl')
        dq_fd = approx_fprime(q, f, grad_precision)

        if verbose:
            print('dq_fd: ', np.round(dq_fd, decimals=4))
            print('dq_qdldl: ', np.round(dq_qdldl, decimals=4))

        npt.assert_allclose(dq_fd, dq_qdldl, rtol=rel_tol, atol=abs_tol)
  1. On the intel mac and windows machine that I was testing on, it looks like torch (needed for the nn module) cannot coexist with numpy>=2 in the same environment (nothing to do with osqp, but torch on some platforms is just not ported over to work with numpy>=2 because the latter is too new). Note that there are no dependency errors, just that they refuse to work with each other, and a import torch throws an error in these cases.

    Given that's the case, I feel like the only users who should be subject to this behavior are the ones who do care about running the nn stuff, and who are currently being informed about the torch dependency (in the dev extra). Thus I'm not enforcing numpy>=2 as a runtime requirement (its back to >=1.7 which must have been selected for some reason back in the development process), but in the dev extra, I'm specifying numpy<2 to go along with the torch requirement. Once torch catches up on all platforms to work with numpy>=2, then the latter can be added as a runtime dependency (though I'm not sure why we just wouldn't want to leave it unpinned, since osqp itself doesn't itself interface with numpy, so it should be left to the other dependencies to duel it out).

  2. I hadn't realized this before, but github keeps changing the underlying software on their images while using the same label (windows-2022 for example). 😡 ! A recent change in Visual Studio version that the windows runner use causes it not to compile anything against cuda 11.x versions anymore, so I bumped up the cuda toolkit version on windows to 12.5. While I was at it, I also bumped it up to 12.4 on ubuntu, the latest available to install by nvidia without a lot of hassles. Being newer, this should hopefully give us a few months of peace before it breaks again.

@vineetbansal vineetbansal self-requested a review July 28, 2024 03:22
@vineetbansal
Copy link
Contributor

@imciner2 , @AmitSolomonPrinceton - let me know if you have any concerns. Otherwise I'll merge this later today.

…yond EOL), and thus codegen in unusable on those platforms too
@vineetbansal vineetbansal merged commit 350677c into master Aug 2, 2024
14 checks passed
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.

3 participants