ENH: Warn when reloading numpy or using numpy in sub-interpreter #16241
Conversation
| @@ -12,13 +14,15 @@ def test_numpy_reloading(): | |||
| VisibleDeprecationWarning = np.VisibleDeprecationWarning | |||
| ModuleDeprecationWarning = np.ModuleDeprecationWarning | |||
|
|
|||
| reload(np) | |||
| with assert_warns(UserWarning): | |||
There was a problem hiding this comment.
do we want to skip this for pypy ?
There was a problem hiding this comment.
I dunno, I think it should be fine on PyPy and probably also applies. The sub-interpreter one I do not know/doubt (but its not tested here).
To be honest, I am fine with not doing this at all (and/or at least leaving the sub-interpreter stuff out). But thought I would propose it. It might be a bit too obnoxious.
There was a problem hiding this comment.
To be honest, I am fine with not doing this at all (and/or at least leaving the sub-interpreter stuff out). But thought I would propose it. It might be a bit too obnoxious.
Skipping the test sounds fine to me if thats what you mean.
I dunno, I think it should be fine on PyPy and probably also applies. The sub-interpreter one I do not know/doubt (but its not tested here).
I tried on pypy, wasn't able to run your ctypes example though since pypy ctypes implementation doesnt have ctypes.pythonapi. But it will probably warn just fine.
There was a problem hiding this comment.
The ctypes example should do nothing on pypy, since I do not think pypy supports subinterpreters (yet). The reload example should apply just as much to pypy as to cpython (not sure whether it is possible to reload in pypy).
I think this is fairly straight forward, unless there is some unknown failure mode for the sub-interpreter warning being given spuriously. Otherwise mainly the wording matters? I am not even sure how well that ctypes thing works, which is why I did not try to bake it into a test...
|
Windows is unhappy: |
|
Yes, same fix as for gh-16239 applies, was hoping to fix that once the other one is merged. |
|
Hmmm, maybe I will look into just fixing the user warning case. I am still happy to put this in as is, but if nobody else is enthusiastic about it, or thinks it might scare users too much, that is fine. For the subprocessors, I feel this a good choice to give a warning, to inform users of our policy (which also invites contributions). So as soon as anyone reviews, I am happy to e.g. split it out. |
|
Where are we with this? Needs rebase. |
This adds a warning when the main NumPy module is reloaded with the assumption that in this case objects such as `np.matrix`, `np._NoValue` or exceptions may be cached internally. It also gives a warning when NumPy is imported in a sub-interpreter.
1f97fec to
7628292
Compare
|
I dunno, rebased it. I thought we tended towards just merging this, but it never happened 🤷. The main reason for not doing it would be someone complaining that it is unnecessary noise, but the warnings are probably correct so.... |
|
Let's give it a shot. Thanks Sebastian. |
This adds a warning when the main NumPy module is reloaded
with the assumption that in this case objects such as
np.matrix,np._NoValueor exceptions may be cached internally.It also gives a warning when NumPy is imported in a sub-interpreter.
I have tested this locally, not sure if adding a tes tmakes sense. The PYPY check was just because I assumed PYPY is different here:
The result for sub-interpreter warning is:
And the other warning:
Anyway, just a thought, maybe the warnings are too big, or too small I don't know. Marking as draft because its built to be merged after the
add_newdocscleanup.