Raise RuntimeError instead of a warning when using array API without setting SCIPY_ARRAY_API=1 #29814
Conversation
|
Note that if scipy/scipy#21529 is implemented in some future version of scipy, we can automatically enable scipy's support from scikit-learn when needed instead of raising an exception. |
|
Forgot to update the test. I am on it. |
There was a problem hiding this comment.
LGTM. Thanks @ogrisel
I think it is better this way as it communicates the requirement clearly.
|
Note that this makes the pytest fail conditionally on installing An alternative would be to raise the exception (as we do in this merged PR) but also skip the array API tests if |
|
I don't mind the current failure. It forces us to enable the flag for scipy, and I just set it in my profile and never think about it again. |
|
I think having to set this environmental variable adds another obstacle for new contributors. To navigate this error message, people first need to know how to translate " Issues, that you can only navigate around if you know how to:
If the IDE doesn't know about These are a lot of possible issues that new contributors might run into. If they also run into them if they don't have array_api libraries installed, then I would hope there is another solution. |
|
I think new contributors wouldn't see this since they wouldn't install For us, we can fix the issue by having a $ cat .env
SCIPY_ARRAY_API=1This would be picked up by |
|
And it seems you then also need to configure your path to .env as Mentioning this because it seems that's not an easy solution neither. And when you have a path like this in your VSCode settings and then switch to another project which also uses an .env file? |
|
@StefanieSenger if you name the file |
Hm, well ... I did name my file .env and had put it on the top layer, outside of the sklearn package. Not sure why it's not working. |
Fixes the UX problem described in #29549 by raising an exception instead of a warning.
In particular the warning is invisible when running pytest and is leading to a non-explicit error. @adrinjalali faced it again recently triggering this PR.
The alternative would be to skip individual scikit-learn tests where we know that scipy array API support is actually needed but this would require extra care when reviewing array API PRs.