Skip to content
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

Hooks: numpy: Simplify numpy hook before sending it to numpy. #5168

Merged
merged 1 commit into from Jan 28, 2021

Conversation

bwoodsend
Copy link
Member

@bwoodsend bwoodsend commented Sep 14, 2020

This is the hook I intend to submit this hook to numpy/numpy/17184 but we'll have to keep a copy to support numpy<1.20.0 at least until those older numpys are considered obsolete. I'd also like your feedback before sending it.

From the commit message

Whilst I'm here I also:

  • Exclude numpy's testing and C/Fortran compiling code, and
    everything else it drags with it (namely scipy).
  • Try to fix Conda support.
  • Avoid dragging 400MB of unused MKL DLLs on Conda.

They're all closed due to our no-conda policy but, (for now) fixes #4935, #4968, #5075, #5082, #5019 and #5019. Although likely at the expense of junk DLLs being dragged in.

@Legorooj Legorooj self-requested a review September 15, 2020 07:40
@bwoodsend
Copy link
Member Author

Ok, tests have finished. A couple of Python 3.5s have failed but it was on a pip freeze. I'm guessing pip has just broken 3.5 compatibility prematurely - I remember it happening with 2.7 too. I'll maybe try forcing a slightly older pip to fix them. But the numpy/PyInstaller stuff is happy all round.

@bwoodsend
Copy link
Member Author

I know I have a changelog entry to add and a linter to sort out. But I'd appreciate a review on the hook itself...

Copy link
Member

@Legorooj Legorooj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small things...

PyInstaller/hooks/hook-numpy.py Outdated Show resolved Hide resolved
PyInstaller/hooks/hook-numpy.py Outdated Show resolved Hide resolved
PyInstaller/hooks/hook-numpy.py Outdated Show resolved Hide resolved
PyInstaller/hooks/hook-numpy.py Outdated Show resolved Hide resolved
@bwoodsend
Copy link
Member Author

@Legorooj Would you be opposed to just chucking in something like:

if is_conda:
    logger.error(
        "Anaconda detected. Anaconda restructures NumPy in a way which makes it's binary dependencies "
        "undetectable to PyInstaller. This can lead to unexpected failures during build, runtime, runtime after "
        "your Conda environment is deactivated, or on moving your compiled app to a different machine. "
        "If you encounter any of these issues then please switch to regular Python.
    )

@bwoodsend bwoodsend force-pushed the fix-numpy-hooks branch 2 times, most recently from 5c0b1ab to 64e1a71 Compare September 17, 2020 14:21
@bwoodsend
Copy link
Member Author

OK, I think I've resolved all @Legorooj's comments. And I've added an exception for .pbds to fix @rokm's issue - although it feels like I'm trying to fix a collapsing roof with sellotape and pritstick. I'm going to fire up the CI machine again. I think it's very unlikely I've not to broken it somewhere...

@bwoodsend
Copy link
Member Author

From CI: Well pip's new incompatibility with Python 3.5 is determined to wind me up but the hook itself doesn't appear to have broken. (yay)

@bwoodsend
Copy link
Member Author

I suppose the goal with the Conda support section is less to actually fix it but to highlight that we are unable to fix it.

Hopefully, once it's in numpy's repo, the Conda people who are responsible for maintaining the Conda numpy will see our additions, see the mess surrounding the Conda stuff and they'd be the ideal people to fix it. They must know what binaries numpy needs to make conda-forge packages. For all we know they may be a

from conda.pkg_util_equivalent import get_package
get_package("numpy").dlls

which they know to use.

@Legorooj
Copy link
Member

@bwoodsend yes, add that in but make it a warning not an error.

Copy link
Member

@Legorooj Legorooj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok but I'd like an approving review from @rokm as well.

@Legorooj
Copy link
Member

Of course fix CI errors.

@bwoodsend
Copy link
Member Author

Oh, I cancelled travis. Travis and AppVeyor passed before my latest Conda warning and lint commits.

@bwoodsend
Copy link
Member Author

@Legorooj What shall we say for the news entry? Regular numpy should be unchanged. Support for non-conda unofficial MKL is new but I doubt many people use that. And I'd say we've battened down some hatches for Conda support but not really fixed it.

@bwoodsend
Copy link
Member Author

Looks like we're still not done. See 3451.

@bwoodsend
Copy link
Member Author

Turns out I'm unable to reproduce the issue flagged in #3451 even after installing the same version of numpy (1.18.5).

@bwoodsend
Copy link
Member Author

bwoodsend commented Sep 28, 2020

Oooh I think I found something much better. There is a metadata list of dlls. It'll probably drag in all kinds of rubbish but no more so than Conda itself already does. I'll look into it but it looks like we may be able to scrap my hacky glob searching in favour of this...

- Add hidden import `numpy.core._dtype_ctypes`.
- Exclude numpy's testing, C/Fortran compiling code and
  scipy which are all `import`ed in NumPy but aren't needed.
- Replace broken special Conda handling with new conda_support
  from  pyinstaller#5213.
@bwoodsend
Copy link
Member Author

This still brings quite a lot of dead weight on Conda. Most the bulk comes from 400MB of mkl DLLs that aren't needed. But I'm reluctant to excluded any of them because this list is almost certainly version dependent and those DLLs may be used by dependents of NumPy like SciPy.

@bwoodsend bwoodsend marked this pull request as ready for review January 12, 2021 16:48
@bwoodsend
Copy link
Member Author

@htgoebel Can we try to get this in before releasing 4.2? It was primarily for this hook that all the Conda stuff from #5213 is needed for.

Comment on lines +50 to +59
excludedimports = [
"scipy",
"pytest",
"nose",
"distutils",
"f2py",
"setuptools",
"numpy.f2py",
"numpy.distutils",
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to figure out how to deal with this in the new modulegraph - where excludedimports are globally respected. IE if the user imports scipy and numpy in the script, scipy won't get bundled. But for now, this is good 👍.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to load dynlib/dll 'D:\\Users\\16886\\Desktop\\molpy\\caffe2_detectron_ops.dll'.
3 participants