-
Notifications
You must be signed in to change notification settings - Fork 80
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
POC: Attempt to package jdaviz in Pyinstaller #1914
Conversation
@pllim found pyinstaller/pyinstaller#1921 (comment), so I tested adding that to our cli.py script, but unfortunately that didn't seem to fix it :( I'm still getting the multiple tabs. Next going to investigate whether I can package just |
@mariobuikhuizen Any idea why |
I noticed
Just wanted to check if this approach is the preferred one, if so, I'm happy to take a look! |
@duytnguyendtn hit roadblocks with all three approaches, but it seemed like this one was the most promising. |
@maartenbreddels Yes! The team would like to explore this Pyinstaller route and I was going to bring it up today at our tagup |
Some notes while getting this working: Due to Python 3.10 I got this message when running the standalone binary:
This was not easy to find, but it appears this requires Python 3.10 The 'loop' behaviour was due to starting a 'kernel' using the `sys.executable', which was set to the binary itself, causing jdaviz to start again and again. This issue was noted here: ipython/ipython#4779 The solutions are to recognize this (see this thread or @astrofrog 's solution https://github.com/astrofrog/voila-qt-app/blob/master/voila_demo.py ) which we use here. |
Superseded by #1960 |
Description
This PR encompasses the learning lessons I've made to package jdaviz in pyinstaller. Requires #1890
Some background:
The goal of pyinstaller is great: it produces either a single executable, or a "portable folder" with an executable that contains everything necessary to run your program. No python, no dependencies, no installing necessary; you just run the executable.
Pyinstaller is "script-oriented", that being it doesn't simply hook into our existing
setuptools.entry_point
s. It effectively runs a.py
file "with no arguments" (quotes will be explained later). To start, I added aif name == '__main__'
flag at the bottom ofcli.py
as the starting point.Pyinstaller encodes all the arguments in a
.spec
configuration file. Thisspec
config file should live "as close" to the script as possible, hence why thecli.spec
file is injdaviz/jdaviz
:Pyinstaller tries to identify all the necessary modules by traversing your script through all its imports and finding all the modules called by all its imports recursively. The side-effect of this approach is the following:
Anything that is not directly imported is not included by default
This can include icons, data files (like our line lists), ALL of our
.vue
files, etc. These need to be listed and included manually. To expedite the development of this PR, any repository with missing assets, I copied over the whole package, rather than try and isolate each and every file that was missing. This by consequence increases the size of the packaged executable. If we were to actually deploy this, care could be taken to locate all the individual files, rather than blindly copying over the entire repo.To build the executable:
This will build the executable in
jdaviz/dist
. It should be as simple as callingjdaviz/dist/cli/cli.exe
This results in the following bug I'm stuck on: the exe enters an infinite loop where it attempts to open multiple copies of jdaviz. None of these copies load properly and just hang on the loading screen:
Console snippet
If you wait long enough, the jupyter client times out waiting for the kernel:
Timeout Traceback
I can't explain why this is happening... My best guess is that pyinstaller is getting confused at which executable it should be calling (we call multiple in the background: jdaviz, voila, jupyter). At this point, I think it would be worth it to pivot to explore other options rather than continue down this route. I'll preserve it here in case we want to pick it up later
Other things to note:
astropy.convolution
C code. This issue went away when moving to astropy devargparse
, hence why I skipped the argument parsing incli._main
and calledcli.main
directly (see the underline). I kept getting aunrecognized arguments: -m path/to/some/file
. Callingcli.main
directly seemed to sidestep this issueChange log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.