replace voila with solara for CLI/standalone app#2909
replace voila with solara for CLI/standalone app#2909kecnry merged 22 commits intospacetelescope:mainfrom
Conversation
kecnry
force-pushed
the
standalone-solara
branch
2 times, most recently
from
5686221 to
2bdd788
Compare
kecnry
force-pushed
the
standalone-solara
branch
2 times, most recently
from
f3d6389 to
a829df0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2909 +/- ##
==========================================
+ Coverage 88.82% 88.84% +0.02%
==========================================
Files 112 113 +1
Lines 17569 17612 +43
==========================================
+ Hits 15605 15647 +42
- Misses 1964 1965 +1 ☔ View full report in Codecov by Sentry. |
kecnry
force-pushed
the
standalone-solara
branch
from
af83044 to
adb00cc
Compare
kecnry
changed the title
* currently has some styling issues and does not support hotreloading properly * still needs some updates to testing, docs, etc
* fix: height and spacing * fix: height of launcher
Use the new class that is also available in a Solara popout window. Keep the styling of #popout-widget-container for backward compatibility with ipypopout<1.3.0.
* feat: use solara for pyinstaller * rich.logging hidden import * make test run * close when tab is closed
could eventually dynamically change this as configs are selected, data loaded, etc.
kecnry
force-pushed
the
standalone-solara
branch
from
a1d3df7 to
9042937
Compare
kecnry
requested review from
bmorris3,
haticekaratay,
javerbukh,
pllim and
rosteen
as code owners
|
Re: Compass crash -- It is not a problem on Also, if threading is a problem, Cubeviz movie maker might also be in trouble. |
|
moving back to draft since there are now likely two merge-blockers (matplotlib issue and pinning ipypopout to a newer version without voila as a dep). Reviews should be able to continue on everything else though! |
bmorris3
left a comment
bmorris3
left a comment
There was a problem hiding this comment.
Launching standalone, loading a spectral cube, and clicking the snackbar logger grays out the interface, before crashing in the terminal with Aborted.:
Any ideas what's up there?
|
nope, but I can reproduce. I'll add to the list of blockers - no obvious traceback or warnings in the console, so this one might be a bit tricky to track down. |
no longer lists voila as a dep
|
from https://docs.python.org/3/library/faulthandler.html - you can set PYTHONFAULTHANDLER=1 to get a stacktrace of when the crash happened, that might help pinpoint the issue. |
Normally, setting MPLBACKEND will set the backend because importing matplotlib will set the backend based on this environment variable. However, in the case where matplotlib is imported before solara, such as jdaviz is doing, we need to set the backend manually. Related: spacetelescope/jdaviz#2909
|
widgetti/solara#741 should fix the matplotlib issue, this was an issue on solara's side (together how the order of importing is happening). |
Can confirm that it fixes it on my end. I'll bump the requirement of solara here once I see that is merged & released. |
Normally, setting MPLBACKEND will set the backend because importing matplotlib will set the backend based on this environment variable. However, in the case where matplotlib is imported before solara, such as jdaviz is doing, we need to set the backend manually. Related: spacetelescope/jdaviz#2909
|
all known issues have now been resolved so I'm marking this as ready for review again. Make sure to update solara to the updated pin of 1.37.2 or later before testing as that fixes the matplotlib/compass issues. |
|
What about the standalone builds? |
These are not currently building on main either. @rosteen - do you think anything here needs to be changed to support that? Does this PR make the situation worse? |
|
I thought Solara would be magical enough to also fix the standalone problems that were choking up voila. Perhaps I misunderstood? |
This comment was marked as resolved.
This comment was marked as resolved.
Can you please help debug as I have not seen that? |
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
Standalone builds still not working aside, I like the code simplification. Thanks!
|
The path stuff... Not sure what happened, I cannot reproduce it now. 🤷♀️ |
bmorris3
left a comment
bmorris3
left a comment
There was a problem hiding this comment.
Thanks for the fixes, this is great!
* replace voila with solara in cli * feat: use solara for pyinstaller * remove no longer needed share/jupyter/nbconvert directories * set page title to "Jdaviz" * override favicon (currently with cubeviz icon) * cli: remove unneeded logic for python<3.10 * update/remove other mentions of voila across codebase * add solara to devdeps * bump ipypopout --------- Co-authored-by: Mario Buikhuizen <[email protected]> Co-authored-by: Maarten Breddels <[email protected]>
6 participants
Description
This pull request replaces the role of voila with solara for launching the standalone/CLI version of jdaviz. Note that we don't currently have a generic (config-agnostic) icon, so this currently uses the cubeviz icon for the page favicon.
Todo before merge:
(likely) closes #2268
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.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.
triviallabel.