WIP, BLD: Test (break) NumPy import from wheel install by obscuring DLL resolution #12994

tylerjereddy · 2019-02-20T03:15:59Z

The simplest & apparently favored fix for failed NumPy imports on (conda) Windows proposed in #12667 is to ensure that requisite DLL(s) are physically present in the numpy/core path.

Before trying a fix that moves the DLL path(s) within a built NumPy wheel, I thought it might be sensible to have a "test" that tries to mimic the conda-related import failures described in the above issue.

Not sure if this is too aggressive, by confining DLL search resolution to system32, but it does seem to nicely generate an ImportError: DLL load failed.

I guess the answer to "is this too aggressive?" is if the physical placement of the DLL(s) at the core path restores the functional numpy import.

tylerjereddy · 2019-02-20T03:23:52Z

Travis missing from CI list it seems -- wonder if that's related to the "draft PR" or something else.

tylerjereddy · 2019-02-20T03:25:10Z

same for circleci -- maybe a longer queue, or related to the draft?

tylerjereddy · 2019-02-20T05:57:41Z

I added a commit with the distutils changes that seemed to restore all-green Azure CI in my debug branch, overcoming the DLL path resolution narrowing from the first commit.

Looks like Shippable really doesn't like this change at first glance! I don't even see suitable error output there.

numpy/distutils/command/build_ext.py

azure-pipelines.yml

mattip · 2019-02-20T07:00:52Z

Closing/Reopening to restart shippable builds

mattip · 2019-02-20T08:52:47Z

We should also remove the code that modifies os.environ['PATH'] here and make sure the linalg module is loading the _umath_linalg c-extension. I am pretty sure it will fail, which means this approach cannot be used. :(

tylerjereddy · 2019-02-20T13:45:15Z

You mean remove the line that I changed instead of changing it? Pretty sure I've adjusted that line you link to in this PR, right?

Travis still not triggered for draft PR it seems, which means codecov isn't either.

tylerjereddy · 2019-02-20T13:49:11Z

If an inappropriate extension is currently being loaded, shouldn't a unit test fail somewhere for linalg?

mattip · 2019-02-20T13:54:24Z

This chunk of code modifies os.environ['PATH'] at startup https://github.com/numpy/numpy/blob/v1.16.1/numpy/distutils/misc_util.py#L2298 should be removed as part of this PR,

I am pretty sure this change will cause linalg to fail to load, since numpy/linalg/_umath_linalg*.pyd also links to the dll we moved into numpy/core

* OpenBLAS-related DLL is now placed in numpy/core instead of numpy/.libs to alleviate issues with DLL resolution that have cropped up in Anaconda environments * added a test in Azure pipelines workflow to mimic the Anaconda DLL resolution mangling

tylerjereddy · 2019-02-20T18:23:38Z

Ok, I removed that chunk of code and everything works just fine for Azure Windows on my branch, so I've pushed the revisions to this branch now & lifted draft status on PR so Travis can run too.

Note that there's ongoing debate between @rgommers & @zooba in the linked issue as to whether this approach is really sustainable for the broader scientific Python ecosystem, though I'm not sure if Ralf is implying we can't do this for NumPy for now.

Anyway, I'll leave this open for now--I'm expecting the CI to pass based on my debug work on another branch, but I guess we'll see.

Not sure if we'd want to make an actual unit test in the test suite instead of a CI stage intervention for "testing" if we do go this route.

rgommers · 2019-02-20T18:54:52Z

numpy/distutils/command/build_ext.py


        # Setup directory for storing generated extra DLL files on Windows
-        self.extra_dll_dir = os.path.join(self.build_temp, '.libs')
+        self.extra_dll_dir = os.path.join(self.build_temp, 'core')


This changes from a generic solution to something that's specific to numpy itself. I think we can do require each package to add some code in combination with changes here, but we can't just make numpy-specific directory layout assumptions here.

I guess if other packages are using package root level .libs folder we'd probably need to retain that default and either have a generic way to offer a customized switch for the DLL storage folder, or bake in a switch if building NumPy only.

scipy-wheels is still on a commit before 473a6f0, so it still has extra-dll rather than .libs. But it looks like we'll need to upgrade that repo to latest numpy master to get the fix for this PATH issue.

rgommers · 2019-02-20T18:56:13Z

though I'm not sure if Ralf is implying we can't do this for NumPy for now.

not sure yet - but as in my other comment, this numpy.distutils code isn't just used by NumPy itself so we need to be clear what other packages like SciPy need to change if/when this PR goes in

mattip · 2019-02-20T19:59:08Z

What if we add a extra_dll directory into site.cfg, and for numpy set it to numpy/core. That would solve our problem at least, I don't know how scipy would deal with it.

tylerjereddy · 2019-02-20T20:18:43Z

What if we add a extra_dll directory into site.cfg, and for numpy set it to numpy/core

I think that falls within the lexicon of my previous suggestion above to:

have a generic way to offer a customized switch for the DLL storage folder

But want to be cautious here.

rgommers · 2019-02-20T23:31:34Z

What if we add a extra_dll directory into site.cfg, and for numpy set it to numpy/core. That would solve our problem at least, I don't know how scipy would deal with it.

Works in principle, however a practical issue is that by default there is no site.cfg and we don't want to start requiring one.

I do think I have a preference for sticking with .libs or extra-dll rather than putting things into core, with the LoadLibrary call. That's for generality, see #12667 (comment). I think that needs to be in numpy.distutils. However if for NumPy itself you both prefer to go with numpy/core, I think that's fine too. That will be the numpy-specific part that then has to not be the default; it can go e.g. in a setup.py file or in some script in the numpy-wheels repo.

rgommers · 2019-02-22T16:45:03Z

This PR does seem to work for NumPy. I think the numpy.distutils discussing will need resolving, however that could be done later. Right now we need to stop the bleeding. What do you think about this plan:

merging this as is
backport to 1.16.x and release 1.16.2 asap
Come back and fix numpy.distutils so it work for SciPy and other packages
backport that fix and put it in 1.16.3 (not very urgent)

That's an extra release to be done, but we're getting multiple bug reports a day that will mostly dry up then, so probably worth it.

tylerjereddy · 2019-02-22T17:08:17Z

I'm just trying the other solution now, but if that doesn't work out swiftly then we may have to I guess

rgommers · 2019-02-22T19:20:13Z

I am pretty sure this change will cause linalg to fail to load, since numpy/linalg/_umath_linalg*.pyd also links to the dll we moved into numpy/core

@mattip this turned out to not be an issue right? reason: numpy.core is imported first and loads the dll already, so numpy/linalg/_umath_linalg*.pyd just picks up the already loaded dll.

charris · 2019-02-22T19:35:00Z

I marked this for backport just so it doesn't get lost.

mattip · 2019-02-23T17:29:38Z

The fix seems good, linalg still works even without the code to modify os.environ['PATH'] so I guess @rgommers's analysis is correct. The plan to release this as 1.16.2 sounds good to me. Similar to linalg, if other packages like scipy needs the dll and imports numpy first it should Just Work, but maybe worth a check before we release this?

tylerjereddy · 2019-02-23T17:46:22Z

This PR will likely break SciPy because of the distutils lib change. The alternate won't.

Each has its own tradeoffs. I think I like the alternate more to reduce impact on the ecosystem by steering clear of distutils.

charris · 2019-03-11T22:51:24Z

Do we want to pursue this test? Anaconda/numpy seems to be working OK now, we have the check in the __init__.py file. There is enough flux in this problem in general that it might be better to wait on developments.

tylerjereddy · 2019-03-11T23:22:52Z

We do have the regression test used in Azure CI here already merged to master with the alternative solution.

I'm still not clear on exactly why either solution only affects conda. I think it is more about likelihood of being helpful--technically it seems like a user could do what I did with the test here / there to break a source install by bricking access to .libs.

But this is so obscure beyond the recent Anaconda event that there's some arguments that the fix should be migrated to the wheels repo only. If we do that, I'd probably want to clone in the modified __init__.py for one of the Azure CI jobs to maintain that guard in dev workflow, although that may be overkill vs. check in nightlies / wheels perhaps.

On the other hand, with Anaconda reversing the design decision that caused the issue in the first place, there's even less justification for the fix.

Can the alternative fix actually do harm? Is guarding against this obscure scenario worth it only for wheels or not even for wheels anymore? I like being defensive against the issue possibly happening again, but there was certainly a wide-ranging set of opinions & some solutions affecting downstream things of course.

Probably want to wait a bit for clear consensus.

rgommers · 2019-03-15T21:59:02Z

Do we want to pursue this test?

I don't care so much about the test, but we do need to fix the problem before it re-occurs; it will likely be reintroduced for Python 3.8 through the Windows Store even if Anaconda stays as is.

I'm still not clear on exactly why either solution only affects conda.

It doesn't only affect conda.

There is enough flux in this problem in general that it might be better to wait on developments.

There's no urgency at the moment I think, but let's mark it for the next release to not completely forget about it.

mattip · 2019-06-11T06:47:16Z

I think we should close this. #13019 fixed the PATH problem by preloading the dll, and the fix was backported in #13041

tylerjereddy · 2019-06-11T19:04:43Z

I'll just ping @rgommers to double check on that. I think I wasn't quite getting the full picture, but maybe the "sub-optimal" approach is indeed ok.

rgommers · 2019-06-12T12:19:16Z

I think the current approach that's in master is probably fine. However there are still two problems that needs solving before the next release: #13019 (comment)

Could one of you tackle those before closing this (or close this and open a new issue for the todos with the 1.17.0 milestone)?

charris · 2019-06-28T18:34:51Z

@mattip is this still needed/wanted?

mattip · 2019-06-28T20:34:10Z

Closing. This was resolved and is tested differently

rgommers · 2019-12-03T01:01:40Z

actually closing now

tylerjereddy added component: distribution 25 - WIP labels Feb 20, 2019

tylerjereddy commented Feb 20, 2019

View reviewed changes

numpy/distutils/command/build_ext.py Outdated Show resolved Hide resolved

tylerjereddy commented Feb 20, 2019

View reviewed changes

azure-pipelines.yml Outdated Show resolved Hide resolved

mattip closed this Feb 20, 2019

mattip reopened this Feb 20, 2019

tylerjereddy force-pushed the windows_conda_dll_handling branch from 04b5dff to fbb93eb Compare February 20, 2019 18:14

tylerjereddy marked this pull request as ready for review February 20, 2019 18:15

tylerjereddy removed the 25 - WIP label Feb 20, 2019

rgommers reviewed Feb 20, 2019

View reviewed changes

tylerjereddy changed the title ~~BLD: Test (break) NumPy import from wheel install by obscuring DLL resolution~~ WIP, BLD: Test (break) NumPy import from wheel install by obscuring DLL resolution Feb 20, 2019

tylerjereddy added the 25 - WIP label Feb 20, 2019

mattip mentioned this pull request Feb 22, 2019

ENH: improve error reporting on windows when numpy fails to load #13014

Closed

tylerjereddy mentioned this pull request Feb 22, 2019

BLD: Windows absolute path DLL loading #13019

Merged

charris added the 09 - Backport-Candidate PRs tagged should be backported label Feb 22, 2019

charris added this to the 1.16.2 release milestone Feb 22, 2019

charris added the 29 - Intel/Anaconda label Feb 22, 2019

charris removed this from the 1.16.2 release milestone Feb 26, 2019

charris mentioned this pull request Feb 26, 2019

BLD: Windows absolute path DLL loading #13041

Merged

rgommers added this to the 1.17.0 release milestone Mar 15, 2019

mattip mentioned this pull request Jun 12, 2019

BUILD: clean up handling of dll loading on Windows #13764

Closed

charris removed this from the 1.17.0 release milestone Jun 14, 2019

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jul 14, 2019

rgommers closed this Dec 3, 2019

Uh oh!

WIP, BLD: Test (break) NumPy import from wheel install by obscuring DLL resolution #12994

WIP, BLD: Test (break) NumPy import from wheel install by obscuring DLL resolution #12994

Uh oh!

Conversation

tylerjereddy commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylerjereddy commented Feb 20, 2019

Uh oh!

tylerjereddy commented Feb 20, 2019

Uh oh!

tylerjereddy commented Feb 20, 2019

Uh oh!

Uh oh!

Uh oh!

mattip commented Feb 20, 2019

Uh oh!

mattip commented Feb 20, 2019

Uh oh!

tylerjereddy commented Feb 20, 2019

Uh oh!

tylerjereddy commented Feb 20, 2019

Uh oh!

mattip commented Feb 20, 2019

Uh oh!

tylerjereddy commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgommers Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

tylerjereddy Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

rgommers Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

rgommers commented Feb 20, 2019

Uh oh!

mattip commented Feb 20, 2019

Uh oh!

tylerjereddy commented Feb 20, 2019

Uh oh!

rgommers commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgommers commented Feb 22, 2019

Uh oh!

tylerjereddy commented Feb 22, 2019

Uh oh!

rgommers commented Feb 22, 2019

Uh oh!

charris commented Feb 22, 2019

Uh oh!

mattip commented Feb 23, 2019

Uh oh!

tylerjereddy commented Feb 23, 2019

Uh oh!

charris commented Mar 11, 2019

Uh oh!

tylerjereddy commented Mar 11, 2019

Uh oh!

rgommers commented Mar 15, 2019

Uh oh!

mattip commented Jun 11, 2019

Uh oh!

tylerjereddy commented Jun 11, 2019

Uh oh!

rgommers commented Jun 12, 2019

Uh oh!

charris commented Jun 28, 2019

Uh oh!

mattip commented Jun 28, 2019

Uh oh!

rgommers commented Dec 3, 2019

Uh oh!

Reviewers

Assignees

Labels

tylerjereddy commented Feb 20, 2019 •

edited

Loading

tylerjereddy commented Feb 20, 2019 •

edited

Loading

rgommers commented Feb 20, 2019 •

edited

Loading