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 skimage fix #565

Closed
wants to merge 3 commits into from
Closed

Conversation

ryandeon
Copy link

starting skimage 0.20, I believe, many things are lazy-loaded. To get my project with skimage to work with pyinstaller, I had to upgrade lazy_loader -> 0.2 and include this hook file. hook file created by reading scikit-image/scikit-image#6772 (comment) and guessing.

Tests did not run for me. There is already a test_skimage in the suite, but pytest -k 'test_skimage' would either deselect all tests or (if I removed some decorators in front of test_skimage) run the function with a bunch of parameters that are not listed in the parametrize mark. I haven't used pytest before, so maybe someone else can figure it out.

starting skimage 0.20, I believe, many things are lazy-loaded.
To get my project with skimage to work with pyinstaller, I had to upgrade lazy_loader -> 0.2 and include this hook file.
hook file created by reading scikit-image/scikit-image#6772 (comment) and guessing
Tests did not run for me. There is already a test_skimage in the suite, but `pytest -k 'test_skimage'` would either deselect all tests or (if I removed some decorators in front of test_skimage) run the function with a bunch of parameters that are not listed in the parametrize mark.
@ryandeon ryandeon marked this pull request as ready for review March 31, 2023 13:08
@rokm
Copy link
Member

rokm commented Mar 31, 2023

Hmmm, while this undoubtedly sorts out all the problems with missing modules and data files, I would prefer to take a more fine-grained approach and not brute-force collect everything.

At a quick glance, the lazy_loader is used by skimage.data and skimage.filters (as well as in skimage itself, but it should be less problematic there except for having to collect the .pyi file). And there's new data that needs to be collected by skimage.morphology.

@ryandeon
Copy link
Author

I definitely understand wanting to avoid overkill. I have some reasons for it, one bad and ...well, you can judge the others.
bad one: I didn't know which submodules might be lazy_loaded or not, I haven't really peered into the internals of skimage.
reason 2) if the hook targets just the ones that are currently lazy_loaded, I imagine that with each version, as skimage updates more submodules, they will convert more and more to using this new way. So the hook will have to be updated in lockstep. I don't actually know what their roadmap is, I'm just being pessimistic about it.
reason 3) I didn't find it added much to the overall build time in pyinstaller. If there are other reasons than wasted time to avoid this kind of overkill, let me know.

@ryandeon
Copy link
Author

that being said, if you'd still prefer I pare it down to just the imports you mention, I can probably do that without a ton more effort.

@rokm
Copy link
Member

rokm commented Mar 31, 2023

I definitely understand wanting to avoid overkill. I have some reasons for it, one bad and ...well, you can judge the others. bad one: I didn't know which submodules might be lazy_loaded or not, I haven't really peered into the internals of skimage.

Yeah, that's fair, and totally fine when you're working around the issue in your application. For hooks that we supply, though, I prefer informed decisions, which sometimes require digging into packages' internals.

reason 2) if the hook targets just the ones that are currently lazy_loaded, I imagine that with each version, as skimage updates more submodules, they will convert more and more to using this new way. So the hook will have to be updated in lockstep. I don't actually know what their roadmap is, I'm just being pessimistic about it.

True, and this is a perennial problem. Not just with lazy-loading, but also with cythonized extensions (from which we cannot infer imports).

reason 3) I didn't find it added much to the overall build time in pyinstaller. If there are other reasons than wasted time to avoid this kind of overkill, let me know.

The primary reason is not so much the build time (after all, a single brute-force collect might have less overhead than checking for versions and performing fine-grained collection in individual hooks), but rather the build size. Which covers both unnecessary modules as well as unnecessary data files.

that being said, if you'd still prefer I pare it down to just the imports you mention, I can probably do that without a ton more effort.

I'll prepare my version of PR with fine-grained changes to supersede this one, but I'd ask to you test it with your application, to ensure that I haven't missed anything (that's not covered by the basic import tests).

@ryandeon
Copy link
Author

ryandeon commented Apr 3, 2023

I just tested your changes, and they worked in my project. Thanks for taking this on!

wxx9248 pushed a commit to wxx9248/CIS-Game-Project-2023W that referenced this pull request Apr 11, 2023
…3.2 (#45)

Bumps
[pyinstaller-hooks-contrib](https://github.com/pyinstaller/pyinstaller-hooks-contrib)
from 2023.1 to 2023.2.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/releases">pyinstaller-hooks-contrib's
releases</a>.</em></p>
<blockquote>
<h2>2023.2</h2>
<p>Please see the <a
href="https://www.github.com/pyinstaller/pyinstaller-hooks-contrib/tree/master/CHANGELOG.rst">changelog</a>
for more details</p>
</blockquote>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/blob/master/CHANGELOG.rst">pyinstaller-hooks-contrib's
changelog</a>.</em></p>
<blockquote>
<h2>2023.2 (2023-04-07)</h2>
<p>New hooks</p>
<pre><code>
* Add hooks for ``moviepy.audio.fx.all`` and ``moviepy.video.fx.all``
that
  collect all
corresponding submodules, so that importing ``moviepy.editor`` from
MoviePy
  works
out-of-the-box in the frozen application.
(`[#559](pyinstaller/pyinstaller-hooks-contrib#559)

&lt;https://github.com/pyinstaller/pyinstaller-hooks-contrib/issues/559&gt;`_)
<p>Updated hooks
</code></pre></p>
<ul>
<li>Add automatic increase of recursion limit in the <code>torch</code>
hook to ensure
that
recursion limit is at least 5000 if <code>torch</code> 2.0.0 or later is
detected.

(<code>[#570](pyinstaller/pyinstaller-hooks-contrib#570)
&lt;https://github.com/pyinstaller/pyinstaller-hooks-contrib/issues/570&gt;</code>_)</li>
<li>Extend <code>cv2</code> hook with support for OpenCV built manually
from source
and for OpenCV installed using the official Windows installer. This
support requires PyInstaller &gt;= 5.3 to work properly.
(<code>[#557](pyinstaller/pyinstaller-hooks-contrib#557)
&lt;https://github.com/pyinstaller/pyinstaller-hooks-contrib/issues/557&gt;</code>_)</li>
<li>Update <code>scikit-image</code> hooks for compatibility with the
0.19.x series;
account for lazy module loading in <code>skimage.filters</code>.
(<code>[#565](pyinstaller/pyinstaller-hooks-contrib#565)
&lt;https://github.com/pyinstaller/pyinstaller-hooks-contrib/issues/565&gt;</code>_)</li>
<li>Update <code>scikit-image</code> hooks for compatibility with the
0.20.x series;
account for switch to <code>lazy_module</code> in
<code>skimage.data</code> and
<code>skimage.filters</code> as well as in main package. Collect new
data files
that are now required by <code>skimage.morphology</code>.
(<code>[#565](pyinstaller/pyinstaller-hooks-contrib#565)
&lt;https://github.com/pyinstaller/pyinstaller-hooks-contrib/issues/565&gt;</code>_)</li>
<li>Update the hook for <code>tensorflow</code> to be compatible with
TensorFlow 2.12.

(<code>[#564](pyinstaller/pyinstaller-hooks-contrib#564)
&lt;https://github.com/pyinstaller/pyinstaller-hooks-contrib/issues/564&gt;</code>_)</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/commit/c317371a98ccb03f202774142fe6c51c72b42c94"><code>c317371</code></a>
Release v2023.2</li>
<li><a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/commit/af3272702eeeddfd7a19b3bdb4b39436adfa8d5c"><code>af32727</code></a>
hooks: torch: automatically increase recursion limit for torch &gt;=
2.0.0 (<a
href="https://redirect.github.com/pyinstaller/pyinstaller-hooks-contrib/issues/570">#570</a>)</li>
<li><a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/commit/988208a7c187cb500c3a607f59e5dd5fdb84b9f1"><code>988208a</code></a>
Scheduled weekly dependency update for week 14 (<a
href="https://redirect.github.com/pyinstaller/pyinstaller-hooks-contrib/issues/569">#569</a>)</li>
<li><a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/commit/43c1eb1f201720311d5bb03c37c2648639ea2d5f"><code>43c1eb1</code></a>
tests: add scikit-image to requirements-test-libraries.txt</li>
<li><a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/commit/f25eae9dbee3f4812ad66af42073fa46af55f350"><code>f25eae9</code></a>
tests: test_skimage: drop test for skimage.viewer</li>
<li><a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/commit/113512f2df1272bdfba63fc5d3bc835cc61abf10"><code>113512f</code></a>
hooks: update scikit-image hooks for compatibility with v0.20.0</li>
<li><a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/commit/942843e6d83d89a8120fd8a1410ffe28e9c85d53"><code>942843e</code></a>
hooks: update scikit-image hooks for compatibility with 0.19.x
series</li>
<li><a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/commit/33ea7ac018498075caf36dd0f97b81477e9f31fd"><code>33ea7ac</code></a>
tests: add scikit-learn to requirements-test-libraries</li>
<li><a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/commit/f8aba9f04f7700493eafc6670d9a01eb00f54031"><code>f8aba9f</code></a>
ci: conditionally enable slow tests for scikit-image and
scikit-learn</li>
<li><a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/commit/fc1d009a7836609219fa41cf47e47b65144d9325"><code>fc1d009</code></a>
Oneshot CI: Add option to pass flags to pytest.</li>
<li>Additional commits viewable in <a
href="https://github.com/pyinstaller/pyinstaller-hooks-contrib/compare/2023.1...2023.2">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pyinstaller-hooks-contrib&package-manager=pip&previous-version=2023.1&new-version=2023.2)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

None yet

2 participants