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

POC: Attempt to package jdaviz in Pyinstaller (take 2) #1960

Merged
merged 89 commits into from
Jul 5, 2023

Conversation

maartenbreddels
Copy link
Collaborator

@maartenbreddels maartenbreddels commented Jan 10, 2023

I can't push to #1914 so opening this PR so I can see how CI runs.

Note that I don't have it fully working yet locally, I can't get Voila to render the widgets yet.

Fix #674

@codecov
Copy link

codecov bot commented Jan 10, 2023

Codecov Report

Patch coverage: 69.49% and project coverage change: -0.91 ⚠️

Comparison is base (1174347) 91.77% compared to head (946780b) 90.86%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1960      +/-   ##
==========================================
- Coverage   91.77%   90.86%   -0.91%     
==========================================
  Files         150      152       +2     
  Lines       16847    17075     +228     
==========================================
+ Hits        15461    15515      +54     
- Misses       1386     1560     +174     
Impacted Files Coverage Δ
...z/configs/imviz/plugins/coords_info/coords_info.py 94.59% <ø> (-0.55%) ⬇️
jdaviz/configs/imviz/plugins/viewers.py 94.93% <ø> (+0.63%) ⬆️
jdaviz/core/template_mixin.py 92.86% <ø> (ø)
jdaviz/core/launcher.py 15.62% <15.62%> (ø)
jdaviz/cli.py 27.05% <45.00%> (+6.16%) ⬆️
jdaviz/app.py 85.35% <45.45%> (-8.11%) ⬇️
jdaviz/core/helpers.py 87.17% <50.00%> (ø)
...configs/default/plugins/export_plot/export_plot.py 53.73% <55.33%> (+5.34%) ⬆️
jdaviz/configs/specviz/helper.py 77.16% <75.00%> (+0.11%) ⬆️
...configs/cubeviz/plugins/tests/test_export_plots.py 75.75% <75.75%> (ø)
... and 19 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@maartenbreddels maartenbreddels force-pushed the pyinstaller_v2 branch 7 times, most recently from 5c2eba7 to 37420b5 Compare January 17, 2023 15:45
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I wonder if there is a way we can incorporate this into CI like https://github.com/astropy/astropy/blob/c5075845d900bc062b633369f228ea8fd89470d6/tox.ini#L153

Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

I now have the Github secrets set up for this to run successfully in our repository (see successful run at https://github.com/spacetelescope/jdaviz/actions/runs/5466039021). We'll need follow-up to integrate some of the other recent launcher work, but I think this is good to be merged now and we can open another PR to improve this later.

@rosteen rosteen added the no-changelog-entry-needed changelog bot directive label Jul 5, 2023
Comment on lines +152 to +169
def _specviz():
_main(config='specviz')


def _specviz2d():
_main(config='specviz2d')


def _imviz():
_main(config='imviz')


def _cubeviz():
_main(config='cubeviz')


def _mosviz():
_main(config='mosviz')
Copy link
Collaborator

Choose a reason for hiding this comment

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

@duytnguyendtn do these need to be changed to use layout rather than config after your work?

And @maartenbreddels, are these necessary for the stand-alone to work? I changed the standalone entry point to call main() instead of _imviz(), but I see they are used in setup.cfg although I don't fully understand what those console_scripts entry points are doing there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope! In the launcher PR, the config arg just translates it to layout if provided:

jdaviz/jdaviz/cli.py

Lines 146 to 149 in 9427518

if config is None:
layout = args.layout
else:
layout = config

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent, got it. I'll just leave this alone then!

@rosteen rosteen removed the no-changelog-entry-needed changelog bot directive label Jul 5, 2023
@rosteen rosteen added this to the 3.6 milestone Jul 5, 2023
@rosteen rosteen merged commit 46be84f into spacetelescope:main Jul 5, 2023
This was referenced Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attempt to Package Jdaviz Installation Executables
9 participants