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.

Sign up for GitHub

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

Jump to bottom

PyInstaller Support for SunPy #5224

Merged
merged 28 commits into from
Jun 24, 2021
Merged

PyInstaller Support for SunPy #5224

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b9228cf
PyInstaller Support for SunPy
jeffreypaul15 Apr 18, 2021
c3ebb25
codestyle changes
jeffreypaul15 Apr 18, 2021
17004e1
changelog added
jeffreypaul15 Apr 18, 2021
1f4d945
changed a few comments
jeffreypaul15 Apr 18, 2021
fbb7a63
Added new test hook and absolute imports
jeffreypaul15 Apr 26, 2021
bf4da06
removed license, asd skip if windows and reworked test script to run …
jeffreypaul15 Apr 26, 2021
9924d42
remvoved astroquery data files
jeffreypaul15 Apr 26, 2021
6497950
Merge branch 'master' into sunpy-5207
jeffreypaul15 Apr 26, 2021
56d76e8
codestyle
jeffreypaul15 Apr 26, 2021
b7e2022
Merge branch 'sunpy-5207' of https://github.com/jeffreypaul15/sunpy i…
jeffreypaul15 Apr 26, 2021
081351d
changed required pyinstaller version
jeffreypaul15 Apr 26, 2021
9099bbd
codestyle
jeffreypaul15 Apr 26, 2021
e6d6991
add to azure pipelines
jeffreypaul15 Apr 26, 2021
5f8da30
codestyle
jeffreypaul15 Apr 26, 2021
93a1c4f
added import and changed name of test
jeffreypaul15 Apr 27, 2021
9221125
codestyle test fix
jeffreypaul15 Apr 27, 2021
555e993
re-added extras=['all'] and updated to use PyInstaller from git
jeffreypaul15 Apr 28, 2021
cfb5c9f
changed deps for pyinstaller
jeffreypaul15 Apr 28, 2021
e4716a6
Update changelog/5224.feature.rst
jeffreypaul15 Apr 28, 2021
2ac7896
Merge branch 'master' of https://github.com/jeffreypaul15/sunpy into …
jeffreypaul15 May 4, 2021
288e46c
removed import to see error
jeffreypaul15 May 4, 2021
d0085e0
redid changes
jeffreypaul15 May 7, 2021
b936a28
hidden import for skimage
jeffreypaul15 May 7, 2021
5be57b2
Merge branch 'main' of https://github.com/sunpy/sunpy into sunpy-5207
jeffreypaul15 May 8, 2021
c1a0288
removed skimage hiddenimport and installed hooks from github
jeffreypaul15 May 8, 2021
8b92179
modified hooks for sunpy and separated test imports
jeffreypaul15 May 8, 2021
eadc990
modified hooks for sunpy and separated test imports
jeffreypaul15 May 8, 2021
21580e0
rebased
jeffreypaul15 Jun 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,3 +230,7 @@ baseline
.history
*.orig
.tmp

# PyInstaller Files
.pyinstaller/run_sunpy_tests
.pyinstaller/run_sunpy_tests.spec
17 changes: 17 additions & 0 deletions .pyinstaller/hooks/hook-sunpy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# ------------------------------------------------------------------
# Copyright (c) 2020 PyInstaller Development Team.
#
# This file is distributed under the terms of the GNU General Public
# License (version 2.0 or later).
#
# The full license is available in LICENSE.GPL.txt, distributed with
# this software.
#
# SPDX-License-Identifier: GPL-2.0-or-later
# -----------------------------------------------------------------

from PyInstaller.utils.hooks import collect_data_files, collect_submodules

# Importing Non-python files
datas = collect_data_files('sunpy')
hiddenimports = collect_submodules('sunpy')
4 changes: 4 additions & 0 deletions .pyinstaller/run_sunpy_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import os
import sys

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

@bwoodsend bwoodsend Apr 21, 2021 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your help on this @bwoodsend.

1 change: 1 addition & 0 deletions changelog/5224.feature.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added hook file and tests for using PyInstaller with SunPy.
jeffreypaul15 marked this conversation as resolved.
Show resolved Hide resolved
31 changes: 31 additions & 0 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,3 +179,34 @@ install_command = pip install --no-deps {opts} {packages}
commands =
conda list
{env:PYTEST_COMMAND} {posargs}

[testenv:pyinstaller]
nabobalis marked this conversation as resolved.
Show resolved Hide resolved
# 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah sorry I undid these changes accidentally.

# 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@nabobalis nabobalis Apr 28, 2021 •
edited

Choose a reason for hiding this comment

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

Do we still need all of these now?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It should, I wonder what the cause is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

commands =
pyinstaller --onefile run_sunpy_tests.py \
--distpath . \
Cadair marked this conversation as resolved.
Show resolved Hide resolved
--additional-hooks-dir hooks \
--exclude-module tkinter \
--hidden-import pytest \
--hidden-import pytest_doctestplus.plugin \
--hidden-import pytest_mpl.plugin
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to create the executable file


./run_sunpy_tests
Copy link
Contributor

Choose a reason for hiding this comment

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

How much of this do we need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@nabobalis nabobalis Apr 18, 2021 •
edited

Choose a reason for hiding this comment

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

Each command, the mpl version, the pyinstaller version etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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...

Cadair marked this conversation as resolved.
Show resolved Hide resolved