-
Notifications
You must be signed in to change notification settings - Fork 40
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
Can you please undo scipy pinning? Faulty test case could be disabled #149
Comments
Which version of OSQP are you using in the environment? |
Thanks for having a look at this. Just installed fresh from |
Hi @enzbus. Thanks for looking into this. Unfortunately, that scipy version had a serious bug in the 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. |
Thanks @bstellato for providing extra context. I just looked into it; the comment by Scipy maintainer scipy/scipy#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 However, I can't find information on how Scipy For more context on my side, Scipy Unless you have a stronger reason than a few broken tests, I would suggest unpinning Scipy, please 🙏 |
I actually already had a fix queued for this in #148, which just moves the pin from the main deps to the optional dependency list 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. |
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!
The text was updated successfully, but these errors were encountered: