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

Fix hook for imageio #396

Merged
merged 7 commits into from
Mar 15, 2022
Merged

Fix hook for imageio #396

merged 7 commits into from
Mar 15, 2022

Conversation

paulmueller
Copy link
Contributor

@paulmueller paulmueller commented Mar 15, 2022

This update makes sure that all plugins from imageio are included. They are imported lazily and thus not found / ignored by PyInstaller. Note that the plugins don't include any binary files, they just contain recipes.

https://imageio.readthedocs.io/en/stable/user_guide/freezing.html

imageio/imageio#766

Please squash-merge this PR and sorry for the messy git history ❤️.

@paulmueller paulmueller marked this pull request as ready for review March 15, 2022 09:15

# imageio plugins are imported lazily since version 0.11.0.
# They are very light-weight, so we can safely include all of them.
hiddenimports = collect_submodules('imageio.plugins')
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how this interacts with transitive dependencies. Will this line only include the plugin (a single .py file inside imageio/plugins), or will this also add transitive dependencies/imports?

If it only adds the plugin file (shallow tracking) these are indeed lightweight, but this won't fully solve the problem, because backends may be left out. In this case I think it would be useful to document that users will have to explicitly add plugin-specific transitive dependencies (imageio-ffmpeg, tiffile, and consorts) as needed.

If the line does track and add transitive dependencies there are two points that come to mind:

  • Firstly, the plugin itself is leight-weight, but its transitive dependencies may not be so (thinking about ffmpeg, the freeImage library, etc. here). In this case it is possible to make a case for more selective importing depending on the use-case.
  • Secondly, does pyinstaller gracefully handle missing modules (ImportError during module loading)? This is an implementation detail of ImageIO, but the way it currently works is that a plugin tries to load the backend (import fancy_backend) and if that fails, the import error is propagated to ImageIO core and properly resolved. If pyinsaller hooks into this process and attempts to discover transitive dependencies by importing plugin modules it, too, will see ImportErrors for pluings with missing backends and will have to have a way to handle with this.

Copy link
Contributor Author

@paulmueller paulmueller Mar 15, 2022

Choose a reason for hiding this comment

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

I will comment point-by-point_

  • You are correct. E.g. "imageio_ffmpeg" will automatically get included. A better solution would be to include the imageio.plugins module without any imports specified therein. (see my next comment further below)
  • Yes, missing imports are usually ignored by PyInstaller.

Copy link
Contributor Author

@paulmueller paulmueller Mar 15, 2022

Choose a reason for hiding this comment

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

I just checked, according to https://github.com/pyinstaller/pyinstaller/blob/develop/PyInstaller/utils/hooks/__init__.py#L545, modules that cannot be imported will issue a warning and will then not be used. That means that if you don't have imageio_ffmpeg installed when you run PyInstaller, you will not have an ffmpeg binary in your distribution directory. I think this is fine and aligns well with how PyInstaller is meant to work.
But maybe someone from the PyInstaller developers would like to comment here as well.

Copy link
Member

Choose a reason for hiding this comment

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

If dependencies are present while freezing, they end up being pulled into the bundle. If not, they aren't, and the code trying to import the missing dependency will end up with a ModuleNotFoundError, as it would in unfrozen python.

So someone using PyInstaller in a minimum build environment (i.e., with minimal requirements installed) ends up with a minimal build, while someone using a "universal" environment with lots of packages installed may end up with a bloated build. But as already noted above, that's how PyInstaller works.

As far as I can tell, this PR does not actually change the behavior w.r.t. collection of plugins' dependencies; the only change is that prior to 2.11.0, the plugins themselves were (all) automatically collected due to static imports in imageio.plugins, while from 2.11.0 they need to be collected using collect_submodules due to lazy imports. But in both cases, the transitive dependencies are collected if they are present.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rokm Thanks for chiming in and explaining; this resolves my doubts.

What I was worried about is that collect_submodules would iterate over the submodules and collect, say, imageio.plugins.ffmpeg (which we assume doesn't have its backend installed), face an ImportError while trying to discover imageio.plugins.ffmpeg's dependencies and halt with exception (since we told it explicitly to collect that module). If that doesn't happen then things should be fine. I don't know if, in this case, imageio.plugins.ffmpeg ends up as part of the build or not, but ImageIO should be robust enough to deal with either situation.


With this said, I have no further things to add to this PR. Once this PR is merged I will find some time to update the ImageIO docs on freezing and add a note on import behavior and a recommendation to build inside a venv if possible.

Co-authored-by: Sebastian Wallkötter <sebastian@wallkoetter.net>
@bwoodsend bwoodsend merged commit 9c23ff0 into pyinstaller:master Mar 15, 2022
@paulmueller paulmueller deleted the fix-hook-for-imageio branch March 15, 2022 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants