search

osqp / osqp-python

Python interface for OSQP
https://osqp.org/
Apache License 2.0
109 stars 41 forks source link

Can you please undo scipy pinning? Faulty test case could be disabled #149

Closed enzbus closed 2 weeks ago

enzbus commented 3 months ago

Hello developers, thanks for making OSQP available to the community.

I just noticed that OSQP is requiring a version of Scipy different that 1.12.0, in this line.

If the comment there is accurate, wouldn't it be better to disable the faulty test case when running in an environment with Scipy 1.12? I'm saying this because this OSQP requirement is forcing an update in an environment I manage which I think is unnecessary and may cause other issues.

If you are open to this I can go on and make a pull request.

Thanks!

imciner2 commented 3 months ago

Which version of OSQP are you using in the environment?

enzbus commented 3 months ago

Which version of OSQP are you using in the environment?

Thanks for having a look at this. Just installed fresh from pip yesterday on Linux, so, the latest one. Should be 0.6.7.post0. Is there a new one?

bstellato commented 3 months ago

Hi @enzbus. Thanks for looking into this. Unfortunately, that scipy version had a serious bug in the scipy.random function. https://github.com/scipy/scipy/issues/20027 We had multiple discussions about it: https://github.com/osqp/osqp-python/issues/121, https://github.com/cvxpy/cvxpy/issues/2341

I do not think it is a good idea to change the tests and even write exceptions to avoid some of them in order to support an old scipy version with bugs. In addition, every example in the osqp docs that generates scipy random matrices is broken for scipy 1.12.0.

This issue has been resolved in scipy since version 1.13.0 (see release notes). Unless there are particular reasons for which many users must support 1.12 (instead of newer 1.13 or 1.14), I would prefer not to make such changes for better code simplicity and long term maintainability.

enzbus commented 3 months ago

Thanks @bstellato for providing extra context. I just looked into it; the comment by Scipy maintainer https://github.com/scipy/scipy/pull/20314 mentions that they don't guarantee Scipy random functions will provide same outputs across versions, so if I may suggest, it would be safer to save the inputs to your testcases as .npz files like you save the outputs.

However, I can't find information on how Scipy 1.12 broke anything in OSQP other than few testcases that were relatively easy to fix, see attached pull request. Am I missing something?

For more context on my side, Scipy 1.12 is currently shipped by the Debian packaging team in testing, will be there for about a year, and there is chance that it may end up in stable. So, because of this requirement any Python environment that contains CVXPY cannot use native Scipy packages, which brings a host of other issues (potentially, also on Debian derivatives like Ubuntu LTS).

Unless you have a stronger reason than a few broken tests, I would suggest unpinning Scipy, please 🙏

imciner2 commented 3 months ago

I actually already had a fix queued for this in https://github.com/osqp/osqp-python/pull/148, which just moves the pin from the main deps to the optional dependency list dev instead. However, that branch is 1.0 only because the master branch is currently setup for the 1.0 beta development and not the current stable releases.

0.6 is using setuptools instead of scikitbuild, so we have a bit of a complicated packaging structure there (we cleaned it up quite a bit for 1.0). I might be able to apply a similar fix to 0.6, but I need to sit and examine our build flow for it to understand better how to make it minimally invasive.

enzbus commented 3 months ago

I actually already had a fix queued for this in https://github.com/osqp/osqp-python/pull/148, which just moves the pin from the main deps to the optional dependency list dev instead. However, that branch is 1.0 only because the master branch is currently setup for the 1.0 beta development and not the current stable releases.

0.6 is using setuptools instead of scikitbuild, so we have a bit of a complicated packaging structure there (we cleaned it up quite a bit for 1.0). I might be able to apply a similar fix to 0.6, but I need to sit and examine our build flow for it to understand better how to make it minimally invasive.

Thanks. For what it's worth, #150 does fix the broken tests. Of 11 that failed, I disabled 6 when running with 1.12, and restored 5. Feel free to copy paste that code rather than merge it if you prefer.

vineetbansal commented 2 weeks ago

fixed in release v0.6.7.post1 and thereafter.