PyInstaller Support for SunPy #5224

jeffreypaul15 · 2021-04-18T22:31:03Z

Fixes #5207
Based on AstroPy's hook and testing script : https://github.com/astropy/astropy/blob/main/.pyinstaller/run_astropy_tests.py
I've written a similar hook for SunPy, although the testing script is quite different from the one over at AstroPy.

The hook is at sunpy/.pyinstaller/hooks/hook-sunpy.py which can be loaded by --additional-hooks-dir while running PyInstaller.

Instead of having to create a separate 'sunpy_tests' directory locally (astropy's version),
the executable created from PyInstaller runs :
pytest -p no:warnings --doctest-rst -m "not mpl_image_compare" --pyargs sunpy
which runs all the tests by default through the executable.

Support will have to be added upstream to pyinstaller-hooks-contrib

pep8speaks · 2021-04-18T22:31:06Z

Hello @jeffreypaul15! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers!

Comment last updated at 2021-06-20 13:23:21 UTC

nabobalis · 2021-04-18T22:35:40Z

What needs to be added upstream?

jeffreypaul15 · 2021-04-18T22:36:07Z

What needs to be added upstream?

The Hook support has to be added to PyInstaller directly

nabobalis · 2021-04-18T22:37:21Z

Would that change this PR? Will you be doing that?

jeffreypaul15 · 2021-04-18T22:38:43Z

Would that change this PR? Will you be doing that?

It would remove the hook file from here as it would be added to PyInstaller directly. If this PR is implemented correctly, sure I would love to make the PR over at PyInstaller.

nabobalis · 2021-04-18T22:41:56Z

So this PR adds support but in theory we could move that support upstream and not need this PR? Or we would still need something here?

jeffreypaul15 · 2021-04-18T22:43:48Z

So this PR adds support but in theory we could move that support upstream and not need this PR? Or we would still need something here?

We would still need the test script that is implemented in this PR and the tox environment for it, only hook-sunpy.py would be moved to PyInstaller.

nabobalis · 2021-04-18T22:45:51Z

tox.ini

+[testenv:pyinstaller]
+# Check that astropy can be included in a PyInstaller bundle without any issues. This
+# also serves as a test that tests do not import anything from outside the tests
+# directories with relative imports, since we copy the tests out to a separate directory
+# to run them.
+description = check that astropy can be included in a pyinstaller bundle
+changedir = .pyinstaller
+deps =
+    # The developer version of PyInstaller doesn't work yet because it
+    # requires https://github.com/pyinstaller/pyinstaller/pull/4888 -
+    # so for now we pin to the latest working commit.
+    git+https://github.com/pyinstaller/pyinstaller.git@c111ec5c#egg=PyInstaller
+    # PyInstaller is not yet compatible with Matplotlib 3.3 so we pin
+    # Matplotlib to an older version for now - see the following issue
+    # for more details: https://github.com/pyinstaller/pyinstaller/issues/5004
+    matplotlib<3.3
+extras =
+    tests
+    net
+    all
+commands =
+    pyinstaller --onefile run_sunpy_tests.py \
+                --distpath . \
+                --additional-hooks-dir hooks \
+                --exclude-module tkinter \
+                --hidden-import pytest \
+                --hidden-import pytest_doctestplus.plugin \
+                --hidden-import pytest_mpl.plugin
+
+    ./run_sunpy_tests


How much of this do we need?

What do you mean? I suppose the comments are required right? These issues still persist

Each command, the mpl version, the pyinstaller version etc

The matplotlib issue still persists with PyInstaller so that is required.
Do we need to test pyinstaller with the dev version? Why not just use the latest working commit?

The matplotlib 3.3 incompatibility should have been cleared up in pyinstaller/pyinstaller#5006 and then 3.4 compatibility in pyinstaller/pyinstaller#5568 which is included in PyInstaller 4.3 (which released a few days ago). If you're still seeing breakages then that's quite worrying...

jeffreypaul15 · 2021-04-19T04:09:00Z

Astropy runs the PyInstaller tests on their github daily workflow, would we need to do that as well?

nabobalis · 2021-04-19T07:51:21Z

Yes, we need to make sure this works on our CI somehow.

Cadair · 2021-04-19T09:40:04Z

tox.ini

+description = check that sunpy can be included in a pyinstaller bundle
+changedir = .pyinstaller
+deps =
+    # The developer version of PyInstaller doesn't work yet because it


Is this copied out of astropy? Is this still true for us, this PR linked here is pretty old.

Yeah, I've taken AstroPy as a reference, I think this wouldn't be a problem for Sunpy though.

Ideally if we can test with a released version that would be better.

Oh yeah sorry I undid these changes accidentally.

Cadair

Thanks for working on this @jeffreypaul15

tox.ini

Cadair · 2021-04-19T09:44:27Z

tox.ini

+                --hidden-import pytest \
+                --hidden-import pytest_doctestplus.plugin \
+                --hidden-import pytest_mpl.plugin


Are these just here to make it possible to run the tests?

This is to create the executable file

tox.ini

Cadair · 2021-04-19T09:45:39Z

tox.ini

+extras =
+    tests
+    net
+    all


We don't need these here do we? All we are running from tox is pyinstaller? or does pyinstaller copy from your current env?

Tox needs these to ensure that the tests run fine by itself, PyInstaller then copies the current environment (tox) and creates the executable based on the environment it was running on.

Do we still need all of these now?

all, encompasess net. can we get away with just all and test?

All doesn't seem to load Beautiful Soup which is part of net, this gave me an error locally I might need to check what else is required from tests and add them individually

It should, I wonder what the cause is.

My apologies, must've been some local issue, we don't need net

nabobalis · 2021-04-20T12:31:47Z

If we could get the hook added upstream first, then we can deal with this PR?

jeffreypaul15 · 2021-04-20T12:36:50Z

If we could get the hook added upstream first, then we can deal with this PR?
Sure, shall I make a PR regarding that?

nabobalis · 2021-04-20T12:39:57Z

Sure.

jeffreypaul15 · 2021-04-21T18:23:14Z

Sure.

@bwoodsend in pyinstaller/pyinstaller-hooks-contrib#112 (comment)_ makes a valid point with the hook, but the all the tests and the script revolves around having the dependencies and optional dependencies. I'm not sure if doing it the way AstroPy has is a good idea then.

bwoodsend · 2021-04-21T22:48:36Z

.pyinstaller/run_sunpy_tests.py

+import os
+import sys
+
+sys.exit(os.system('pytest -p no:warnings --doctest-rst -m "not mpl_image_compare" --pyargs sunpy'))


No no no! This will just run regular unfrozen pytest so it's not testing anything except os.system()! Use pytest.main() instead.

Suggested change

sys.exit(os.system('pytest -p no:warnings --doctest-rst -m "not mpl_image_compare" --pyargs sunpy'))

import pytest

# Set some default args.

args = ['-p', 'no:warnings', '--doctest-rst', '-m',

'not mpl_image_compare. '--pyargs sunpy']

# And allow additional arguments to be supplied at runtime.

args += sys.argv[1:]

pytest.main(args)

However, without an import sunpy in there, PyInstaller will have no reason to think that sunpy is a dependency so it won't collect it (or run the hook) unless you force it to with a --hiddenimport=sunpy or put import sunpy in the script.

That's still probably not the end of the story though - it depends: Does import sunpy load everything so that you can access any public corner without additional import statements or are you expected to use from sunpy.foo.bar import xyz to pull the bits you need out? If it's the latter then PyInstaller will consider sunpy.foo.bar to be unneeded unless you add it as a hiddenimport or an explicit reference in run_sunpy_tests.py too. (Note, this won't apply if you opt to keep the collect_submodules("sunpy") in the hook. Using collect_submodules("sunpy") does simplify testing - at the expense of making builds huge.)

It's quite likely that you'll have to have more than one test script, each importing and testing a different corner of sunpy, and each using only the optional dependencies that that corner requires.

Thanks for your help on this @bwoodsend.

jeffreypaul15 · 2021-04-22T13:52:41Z

Sure.

I've uncovered another issue with PyInstaller and SunPy :

from pkg_resources import get_distribution
get_distribution('sunpy')

Doesn't work with PyInstaller, pkg_resources is required for sunpy.util.sysinfo and this causes an issue with creating an executable for sunpy with the required hooks as well.

The 0df8534#diff-0c2fc003b7a8d4cda00792118c87f10505af98697c012a80656f63cb926c88a6 update to sysinfo.py probably uncovered an issue with pyinstaller and pkg_resources, so I'm not sure we can use PyInstaller with SunPy until there's a workaround for this.

Although, PyInstaller can be used with any current stable version of SunPy just before from get_distribution from pkg_resources was added.

bwoodsend · 2021-04-22T16:50:49Z

Oh, that one is quite an easy one. get_distribution('sunpy') requires the package metadata (that sunpy-x.y.z.dist-info folder that pip creates whenever you install sunpy) to be included. Just put in hook-sunpy.py:

from PyInstaller.utils.hooks import copy_metadata
datas = copy_metadata("sunpy")

Of course, as you already have a datas = collect_data_files("sunpy") in there you'd want to combine the two:

datas += copy_metadata("sunpy")

jeffreypaul15 · 2021-04-22T16:52:39Z

Oh, that one is quite an easy one. get_distribution('sunpy') requires the package metadata (that sunpy-x.y.z.dist-info folder that pip creates whenever you install sunpy) to be included. Just put in hook-sunpy.py:
from PyInstaller.utils.hooks import copy_metadata
datas = copy_metadata("sunpy")
Of course, as you already have a datas = collect_data_files("sunpy") in there you'd want to combine the two:
datas += copy_metadata("sunpy")

Thanks for this.

jeffreypaul15 · 2021-04-24T10:30:07Z

@nabobalis what would we want to do here? We can either skip the tests dependencies while making the hook, but this complicates testing, or we could have a separate internal hook within the repository for testing and have the main hook present upstream without the original test dependencies.

nabobalis · 2021-04-24T11:51:46Z

@nabobalis what would we want to do here? We can either skip the tests dependencies while making the hook, but this complicates testing, or we could have a separate internal hook within the repository for testing and have the main hook present upstream without the original test dependencies.

Whatever makes it easier to setup and to ensure that hook works and the offline tests pass.

jeffreypaul15 · 2021-04-24T12:01:05Z

@nabobalis what would we want to do here? We can either skip the tests dependencies while making the hook, but this complicates testing, or we could have a separate internal hook within the repository for testing and have the main hook present upstream without the original test dependencies.

Whatever makes it easier to setup and to ensure that hook works and the offline tests pass.

Offline tests require certain imports like sunpy.net.hek.tests which aren't needed for when a user is using sunpy.
Like mentioned above, including all of these imports would make all the tests run fine but it increases the optional dependencies while making the executable. We have around 154 such imports which aren't required (other than for testing).
Astropy has gone with the approach of having to include everything in the hook and make the tests easier so this PR was based on the same idea, although pyinstaller/pyinstaller-hooks-contrib#112 (comment) is a reason to reconsider that.

nabobalis · 2021-05-12T08:54:30Z

Could this be an upstream ASDF issue?

This seems like an issue with the dev version of PyInstaller. The latest stable release of pyinstaller works fine with the tests but it has the extras=['all'] issue, now that I'm installing the dev version from git the asdf path issue persists. I think we might need to wait for the next stable release for this.

Won't the next stable release be the dev version and then have the same issue?

jeffreypaul15 · 2021-05-12T12:34:41Z

Could this be an upstream ASDF issue?

This seems like an issue with the dev version of PyInstaller. The latest stable release of pyinstaller works fine with the tests but it has the extras=['all'] issue, now that I'm installing the dev version from git the asdf path issue persists. I think we might need to wait for the next stable release for this.

Won't the next stable release be the dev version and then have the same issue?

I'm assuming that they'd fix it before the release, should I report it to them? I can't seem to find the change that caused it.

bwoodsend · 2021-05-12T12:45:59Z

I suspect that this asdf issue is in part due to pyinstaller/pyinstaller#5774. asdf makes use of pkg_resources.require("pyyaml") which requires all metadata to be included recursively for pyyaml and all its dependencies. For some reason we haven't worked out, before pyinstaller/pyinstaller#5774 pkg_resources.require("pyyaml") did nothing - i.e. it was doubly broken in a way that made it look fixed. asdf also requires its own metadata, both for a version and for an entry point. And it's full of yaml files so it needs those collected too. I can get a hello world asdf script running with:

pyinstaller --collect-data asdf --copy-metadata asdf --copy-metadata pyyaml --copy-metadata numpy --copy-metadata semantic-version --copy-metadata pyrsistent --copy-metadata six --copy-metadata attrs --copy-metadata setuptools test.py

although we (PyInstaller) will want to stream line it into something more sane. I'm adding a recursive option to copy metadata to deal with pkg_resources.require("...") issues. And I'm experimenting with scanning bytecode for uses of pkg_resources and importlib.metatdata to see if we can automate metadata completely.

nabobalis · 2021-06-19T11:00:16Z

Where did we get with this again? I forgot.

jeffreypaul15 · 2021-06-19T11:30:38Z

Where did we get with this again? I forgot.

Last I checked, I think the asdf issue still persists, we would be able to use sunpy without asdf as of this current PR

nabobalis · 2021-06-19T22:08:59Z

Will need a rebase due to the file conflict.

jeffreypaul15 · 2021-06-20T12:27:47Z

Will need a rebase due to the file conflict.

On it.

nabobalis · 2021-06-23T16:36:59Z

What will getting pyinstaller/pyinstaller-hooks-contrib#112 merged in do for this PR or will it change nothing?

jeffreypaul15 · 2021-06-24T16:05:40Z

@Cadair
The multiple .fits, .rst, .csv and .txt files are are loaded when we perform collect_data_files("sunpy").
77/136 of these files are required for testing only, I'll open a PR over at PyInstaller for the hook without these files.

jeffreypaul15 · 2021-06-24T16:06:32Z

What will getting pyinstaller/pyinstaller-hooks-contrib#112 merged in do for this PR or will it change nothing?

It won't really do much, since I'm going to open another PR which only contains the required files for sunpy to run without tests.
We do however need to keep in mind that ASDF won't work either way

nabobalis · 2021-06-24T17:24:25Z

What will getting pyinstaller/pyinstaller-hooks-contrib#112 merged in do for this PR or will it change nothing?

It won't really do much, since I'm going to open another PR which only contains the required files for sunpy to run without tests.
We do however need to keep in mind that ASDF won't work either way

Another PR here or upstream?

jeffreypaul15 · 2021-06-24T17:25:43Z

Another PR here or upstream?

Upstream

nabobalis · 2021-06-24T17:26:17Z

Another PR here or upstream?

Upstream

Why another one? Is the one you opened not enough?

jeffreypaul15 · 2021-06-24T17:28:07Z

Another PR here or upstream?

Upstream

Why another one? Is the one you opened not enough?

I closed that one, thought it would be better to create a new one instead.

nabobalis · 2021-06-24T17:31:42Z

So outside of that and asdf, this PR is good to go?

Can you open an issue on the asdf repo about pyinstaller please?

jeffreypaul15 · 2021-06-24T17:37:04Z

So outside of that and asdf, this PR is good to go?

It should be.

Can you open an issue on the asdf repo about pyinstaller please?

Sure

Co-authored-by: Nabil Freij <nabil.freij@gmail.com>

PyInstaller Support for SunPy

b9228cf

codestyle changes

c3ebb25

changelog added

17004e1

nabobalis reviewed Apr 18, 2021

View reviewed changes

changed a few comments

1f4d945

Cadair reviewed Apr 19, 2021

View reviewed changes

nabobalis marked this pull request as draft April 20, 2021 10:07

nabobalis added No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) Infrastructure Issues or PRs that affect the CI or packaging of SunPy labels Apr 20, 2021

bwoodsend reviewed Apr 21, 2021

View reviewed changes

nabobalis marked this pull request as ready for review May 12, 2021 09:13

nabobalis requested review from a team as code owners May 12, 2021 09:13

nabobalis added the Needs Review Needs reviews before merge label May 12, 2021

rebased

21580e0

nabobalis removed request for a team June 21, 2021 09:40

Cadair approved these changes Jun 23, 2021

View reviewed changes

nabobalis approved these changes Jun 24, 2021

View reviewed changes

nabobalis merged commit 53d3cc8 into sunpy:main Jun 24, 2021

jeffreypaul15 mentioned this pull request Jun 27, 2021

Removes duplicate test to be skipped for pyinstaller #5423

Merged

ayshih pushed a commit to ayshih/sunpy that referenced this pull request Sep 15, 2021

PyInstaller Support for SunPy (sunpy#5224)

4bf889e

Co-authored-by: Nabil Freij <nabil.freij@gmail.com>

dstansby mentioned this pull request Oct 21, 2021

Milestones, bugfixes, oh my. #3904

Closed

dstansby removed the Needs Review Needs reviews before merge label Jan 24, 2022

-sys.exit(os.system('pytest -p no:warnings --doctest-rst -m "not mpl_image_compare" --pyargs sunpy'))
+import pytest
+# Set some default args.
+args = ['-p', 'no:warnings', '--doctest-rst', '-m',
+        'not mpl_image_compare. '--pyargs sunpy']
+# And allow additional arguments to be supplied at runtime.
+args += sys.argv[1:]
+pytest.main(args)

PyInstaller Support for SunPy #5224

PyInstaller Support for SunPy #5224

Conversation

jeffreypaul15 commented Apr 18, 2021 • edited

pep8speaks commented Apr 18, 2021 • edited

Comment last updated at 2021-06-20 13:23:21 UTC

nabobalis commented Apr 18, 2021

jeffreypaul15 commented Apr 18, 2021

nabobalis commented Apr 18, 2021

jeffreypaul15 commented Apr 18, 2021

nabobalis commented Apr 18, 2021

jeffreypaul15 commented Apr 18, 2021

Choose a reason for hiding this comment

jeffreypaul15 Apr 18, 2021 • edited

Choose a reason for hiding this comment

nabobalis Apr 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffreypaul15 commented Apr 19, 2021

nabobalis commented Apr 19, 2021

Choose a reason for hiding this comment

jeffreypaul15 Apr 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cadair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabobalis Apr 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabobalis commented Apr 20, 2021

jeffreypaul15 commented Apr 20, 2021

nabobalis commented Apr 20, 2021

jeffreypaul15 commented Apr 21, 2021 • edited

bwoodsend Apr 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffreypaul15 commented Apr 22, 2021 • edited

bwoodsend commented Apr 22, 2021

jeffreypaul15 commented Apr 22, 2021

jeffreypaul15 commented Apr 24, 2021 • edited

nabobalis commented Apr 24, 2021

jeffreypaul15 commented Apr 24, 2021 • edited

nabobalis commented May 12, 2021

jeffreypaul15 commented May 12, 2021

bwoodsend commented May 12, 2021 • edited

nabobalis commented Jun 19, 2021 • edited

jeffreypaul15 commented Jun 19, 2021

nabobalis commented Jun 19, 2021

jeffreypaul15 commented Jun 20, 2021

nabobalis commented Jun 23, 2021

jeffreypaul15 commented Jun 24, 2021 • edited

jeffreypaul15 commented Jun 24, 2021 • edited

nabobalis commented Jun 24, 2021

jeffreypaul15 commented Jun 24, 2021

nabobalis commented Jun 24, 2021

jeffreypaul15 commented Jun 24, 2021

nabobalis commented Jun 24, 2021

jeffreypaul15 commented Jun 24, 2021

jeffreypaul15 commented Apr 18, 2021 •

edited

pep8speaks commented Apr 18, 2021 •

edited

jeffreypaul15 Apr 18, 2021 •

edited

nabobalis Apr 18, 2021 •

edited

jeffreypaul15 Apr 19, 2021 •

edited

nabobalis Apr 28, 2021 •

edited

jeffreypaul15 commented Apr 21, 2021 •

edited

bwoodsend Apr 21, 2021 •

edited

jeffreypaul15 commented Apr 22, 2021 •

edited

jeffreypaul15 commented Apr 24, 2021 •

edited

jeffreypaul15 commented Apr 24, 2021 •

edited

bwoodsend commented May 12, 2021 •

edited

nabobalis commented Jun 19, 2021 •

edited

jeffreypaul15 commented Jun 24, 2021 •

edited

jeffreypaul15 commented Jun 24, 2021 •

edited