BUG: Make sure numpy globals keep identity after reload.

[charris]

Reloading currently causes problems because global classes defined in
numpy/__init__.py change their identity (a is b) after reload. The solution here is to
raise an error when a reload is attempted.

Closes #7844.

[charris]

Note that if we want to preserve the reload option I think that the best thing to do is to move the relevant class definitions into a non-reloadable numpy/_globals.py module.

[njsmith]

@cgohlke: you're the only one I know who has tried reloading numpy recently :-). Would merging this cause any problems for you?

[cgohlke]

This PR might cause pandas.show_versions() not being able to determine the numpy version.

The original bug report came from a Pandas user, who noticed that pandas.show_versions() broke numpy 1.11.x because pandas.show_versions() calls importlib.import_module('numpy'), which does a reload().

[charris]

@cgohlke Hmm, wonder why pandas does that, why not a simple import? Anyway, might as well move the functions to a safe space.

[charris]

OK, should be fixed for pandas.

I suspect it is a bug for pandas to call imp.import_module with its reload feature. Reloading seems to me to be generally dangerous unless packages are carefully designed to have no issues stemming from one module depending on classes defined in another that might be reloaded. OTOH, I suppose one could disallow defining classes in __init__.py functions.

[njsmith]

Yeah, I think pandas is doing this accidentally, and that it's a bug -- I just commented here.

I think it would still be fine for numpy to error out on reload (it just means that pandas.show_versions would show the numpy version as None until they fix their bug), and I guess it would be a more robust long-term solution than the approach currently in this PR where we have to remember in the future never to define anything inside __init__.py. But really so long as pandas fixes their bug I don't think it matters very much whether reload(numpy) fails or works or breaks things, so whatever, LGTM.

[charris]

I note that numpy uses load_module in a few places. We should probably take a closer look at those.

[charris]

@njsmith I prefer making numpy non-reloadable as reloading it might not be what folks expect even if it works. If it doesn't break pandas, great.

[charris]

I went back to the first version, raise an error. There are three versions on hand to choose from: raise and error, explicit imports, new globals file.

[njsmith]

LGTM. Just for due diligence, have you double-checked that it doesn't blow up pandas.show_versions? (I know I said above that it shouldn't based on reading the code, but I didn't actually test it ;-).)

Though I guess since pandas tests against numpy master, we could also just merge and let them tell us if we got it wrong :-)

[charris]

Yes, it works, at least for the sequence in the pandas issue. The numpy version is also correctly reported, so perhaps load_module has a fallback.

[charris]

Let's merge and see how this does.

[WeatherGod]

heads up... this broke some edge cases for matplotlib's build process as noted in matplotlib/matplotlib#6928. I am going to see if we can do without the reload, though I figured I should let all know this happened.

[charris]

@WeatherGod Reload is evil, that's why we broke your build ;) We could fix it on our end, but it would be better to get out of the habit of using reload.

EDIT: An alternative point of view: "isinstance() regarded as harmful".

EDIT: A consequence of: "singletons regarded as harmful".

[mhvk]

Apparently we are reloading numpy in our astropy tests as well -- which we shouldn't and is hopefully easy to fix...

[charris]

Looks like this should maybe just be a (Visible)DeprecationWarning in the Numpy 1.11.2 release. Thoughts?

[charris]

@njsmith @WeatherGod @mhvk Looks like the reload safe alternative might be the best way to handle backwards compatibility. Raising a error certainly brings attention to the problem, but as this will be part of the 1.11.2 release it is likely that too much downstream code will be broken. Maybe make it reload safe and change this to a deprecation?

[mhvk]

I think I agree that it is best to try to continue to have "reload" work, at least sort-of.