WIP: replace voila with solara for CLI/standalone app

[kecnry]

Description

This pull request replaces the role of voila with solara for launching the standalone/CLI version of jdaviz.

This is still a work in progress and not yet ready for review.

• [x] currently has some styling issues (see https://github.com/kecnry/jdaviz/pull/9 and https://github.com/kecnry/jdaviz/pull/12)
• [x] test/update pyinstaller (see https://github.com/kecnry/jdaviz/pull/11
• [x] can we get rid of (or do we need to modify) jdaviz-cli-entrypoint.py? (see https://github.com/kecnry/jdaviz/pull/11)
• [x] reimplement closing kernel when closing browser window(s) (see https://github.com/kecnry/jdaviz/pull/11)
• [ ] does not support hotreloading properly -- needs follow-up
• [ ] still needs some updates to testing, docs, etc
• [ ] page/tab title, favicon, etc

Change log entry

• [ ] Is a change log needed? If yes, is it added to 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, maintainer should 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.

• [ ] Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
• [ ] Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
• [ ] Do the proposed changes follow the STScI Style Guides?
• [ ] Are tests added/updated as required? If so, do they follow the STScI Style Guides?
• [ ] Are docs added/updated as required? If so, do they follow the STScI Style Guides?
• [ ] Did the CI pass? If not, are the failures related?
• [x] Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
• [ ] After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 33.33333% with 50 lines in your changes missing coverage. Please review.

Project coverage is 88.84%. Comparing base (d71dc91) to head (a8443cc). Report is 2 commits behind head on main.

Files	Patch %	Lines
jdaviz/solara.py	41.66%	28 Missing :warning:
jdaviz/cli.py	0.00%	17 Missing :warning:
jdaviz/core/launcher.py	50.00%	5 Missing :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[rosteen]

My first thought is that this is fantastic and I'll never have to worry about the jinja2 template error again. Second thought is that the --layout option from the command line seems to be broken - calling jdaviz --layout cubeviz brings you to the config launch page, unlike on main where it takes you straight to cubeviz.

[pllim]

Was hoping standalone would be magically fixed but I guess not.

Error: Error: Timed out waiting for: tcp:8765

[rosteen]

Was hoping standalone would be magically fixed but I guess not.

Error: Error: Timed out waiting for: tcp:8765

Well, I wouldn't expect the Wait for Voila to get online step to pass now that we're not using Voila...

[pllim]

In that case, that workflow also needs to be updated here?

[pllim]

p.s. How is the ubuntu one passing then?

[rosteen]

In that case, that workflow also needs to be updated here?

Yes, although given that it's already failing we could potentially do that as a separate follow-up I guess.

[rosteen]

p.s. How is the ubuntu one passing then?

Hmm you're right, it looks like the Solara server is running on that same port, so the check is still valid. Might be a legitimate failure in that case.

[kecnry]

the --layout option from the command line seems to be broken

Fixed!

[kecnry]

Well, I wouldn't expect the Wait for Voila to get online step to pass now that we're not using Voila...

This was just the step name in the workflow though, right? I just pushed a commit that updates the port, let's see if that passes 🤞

[rosteen]

Well, I wouldn't expect the Wait for Voila to get online step to pass now that we're not using Voila...

This was just the step name in the workflow though, right? I just pushed a commit that updates the port, let's see if that passes 🤞

Yeah, @pllim pointed out it was passing on Ubuntu.

[pllim]

I opened an image with this branch (Imviz in dark mode, yay). But then it crashed when I try to open the Compass plugin.

Full traceback

jdaviz/configs/imviz/wcs_utils.py:224: UserWarning: Starting a Matplotlib GUI outside of the main thread will likely fail.
  fig, ax = plt.subplots()
ERROR:    Uncaught exception: Traceback (most recent call last):
  File "ipywidgets/widgets/widget.py", line 191, in __call__
    local_value = callback(*args, **kwargs)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "ipyvue/VueTemplateWidget.py", line 60, in _handle_event
    getattr(self, "vue_" + event)(data)
  File "jdaviz/core/template_mixin.py", line 483, in vue_plugin_ping
    self.plugin_opened = True
    ^^^^^^^^^^^^^^^^^^
  File "traitlets/traitlets.py", line 729, in __set__
    self.set(obj, value)
  File "traitlets/traitlets.py", line 718, in set
    obj._notify_trait(self.name, old_value, new_value)
  File "traitlets/traitlets.py", line 1501, in _notify_trait
    self.notify_change(
  File "ipywidgets/widgets/widget.py", line 701, in notify_change
    super().notify_change(change)
  File "traitlets/traitlets.py", line 1513, in notify_change
    return self._notify_observers(change)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "traitlets/traitlets.py", line 1560, in _notify_observers
    c(event)
  File "jdaviz/core/template_mixin.py", line 565, in _update_is_active
    self.is_active = ((len(self.irrelevant_msg) == 0)
    ^^^^^^^^^^^^^^
  File "traitlets/traitlets.py", line 729, in __set__
    self.set(obj, value)
  File "traitlets/traitlets.py", line 718, in set
    obj._notify_trait(self.name, old_value, new_value)
  File "traitlets/traitlets.py", line 1501, in _notify_trait
    self.notify_change(
  File "ipywidgets/widgets/widget.py", line 701, in notify_change
    super().notify_change(change)
  File "traitlets/traitlets.py", line 1513, in notify_change
    return self._notify_observers(change)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "traitlets/traitlets.py", line 1560, in _notify_observers
    c(event)
  File "jdaviz/core/template_mixin.py", line 318, in wrapper
    ret_ = meth(self, msg)
           ^^^^^^^^^^^^^^^
  File "jdaviz/configs/imviz/plugins/compass/compass.py", line 78, in _compass_with_new_viewer
    viewer.on_limits_change()  # Force redraw
    ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "jdaviz/configs/imviz/plugins/viewers.py", line 155, in on_limits_change
    self.set_compass(self.state.layers[i].layer)
  File "jdaviz/configs/imviz/plugins/viewers.py", line 271, in set_compass
    self.compass.draw_compass(image.label, wcs_utils.draw_compass_mpl(
                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "jdaviz/configs/imviz/wcs_utils.py", line 224, in draw_compass_mpl
    fig, ax = plt.subplots()
              ^^^^^^^^^^^^^^
  File "matplotlib/pyplot.py", line 1759, in subplots
    fig = figure(**fig_kw)
          ^^^^^^^^^^^^^^^^
  File "matplotlib/pyplot.py", line 1027, in figure
    manager = new_figure_manager(
              ^^^^^^^^^^^^^^^^^^^
  File "matplotlib/pyplot.py", line 550, in new_figure_manager
    return _get_backend_mod().new_figure_manager(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "matplotlib/backend_bases.py", line 3507, in new_figure_manager
    return cls.new_figure_manager_given_figure(num, fig)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "matplotlib/backend_bases.py", line 3512, in new_figure_manager_given_figure
    return cls.FigureCanvas.new_manager(figure, num)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "matplotlib/backend_bases.py", line 1797, in new_manager
    return cls.manager_class.create_with_canvas(cls, figure, num)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "matplotlib/backends/_backend_tk.py", line 504, in create_with_canvas
    manager = cls(canvas, num, window)
              ^^^^^^^^^^^^^^^^^^^^^^^^
  File "matplotlib/backends/_backend_tk.py", line 457, in __init__
    super().__init__(canvas, num)
  File "matplotlib/backend_bases.py", line 2644, in __init__
    if rcParams['toolbar'] != 'toolmanager':
       ~~~~~~~~^^^^^^^^^^^
  File "solara/server/patch.py", line 191, in __getitem__
    return self._get_context_dict().__getitem__(key)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: 'toolbar'

ERROR:solara.server.patch:Uncaught exception: Traceback (most recent call last):
  File "ipywidgets/widgets/widget.py", line 191, in __call__
    local_value = callback(*args, **kwargs)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "ipyvue/VueTemplateWidget.py", line 60, in _handle_event
    getattr(self, "vue_" + event)(data)
  File "jdaviz/core/template_mixin.py", line 483, in vue_plugin_ping
    self.plugin_opened = True
    ^^^^^^^^^^^^^^^^^^
  File "traitlets/traitlets.py", line 729, in __set__
    self.set(obj, value)
  File "traitlets/traitlets.py", line 718, in set
    obj._notify_trait(self.name, old_value, new_value)
  File "traitlets/traitlets.py", line 1501, in _notify_trait
    self.notify_change(
  File "ipywidgets/widgets/widget.py", line 701, in notify_change
    super().notify_change(change)
  File "traitlets/traitlets.py", line 1513, in notify_change
    return self._notify_observers(change)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "traitlets/traitlets.py", line 1560, in _notify_observers
    c(event)
  File "jdaviz/core/template_mixin.py", line 565, in _update_is_active
    self.is_active = ((len(self.irrelevant_msg) == 0)
    ^^^^^^^^^^^^^^
  File "traitlets/traitlets.py", line 729, in __set__
    self.set(obj, value)
  File "traitlets/traitlets.py", line 718, in set
    obj._notify_trait(self.name, old_value, new_value)
  File "traitlets/traitlets.py", line 1501, in _notify_trait
    self.notify_change(
  File "ipywidgets/widgets/widget.py", line 701, in notify_change
    super().notify_change(change)
  File "traitlets/traitlets.py", line 1513, in notify_change
    return self._notify_observers(change)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "traitlets/traitlets.py", line 1560, in _notify_observers
    c(event)
  File "jdaviz/core/template_mixin.py", line 318, in wrapper
    ret_ = meth(self, msg)
           ^^^^^^^^^^^^^^^
  File "jdaviz/configs/imviz/plugins/compass/compass.py", line 78, in _compass_with_new_viewer
    viewer.on_limits_change()  # Force redraw
    ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "jdaviz/configs/imviz/plugins/viewers.py", line 155, in on_limits_change
    self.set_compass(self.state.layers[i].layer)
  File "jdaviz/configs/imviz/plugins/viewers.py", line 271, in set_compass
    self.compass.draw_compass(image.label, wcs_utils.draw_compass_mpl(
                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "jdaviz/configs/imviz/wcs_utils.py", line 224, in draw_compass_mpl
    fig, ax = plt.subplots()
              ^^^^^^^^^^^^^^
  File "matplotlib/pyplot.py", line 1759, in subplots
    fig = figure(**fig_kw)
          ^^^^^^^^^^^^^^^^
  File "matplotlib/pyplot.py", line 1027, in figure
    manager = new_figure_manager(
              ^^^^^^^^^^^^^^^^^^^
  File "matplotlib/pyplot.py", line 550, in new_figure_manager
    return _get_backend_mod().new_figure_manager(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "matplotlib/backend_bases.py", line 3507, in new_figure_manager
    return cls.new_figure_manager_given_figure(num, fig)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "matplotlib/backend_bases.py", line 3512, in new_figure_manager_given_figure
    return cls.FigureCanvas.new_manager(figure, num)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "matplotlib/backend_bases.py", line 1797, in new_manager
    return cls.manager_class.create_with_canvas(cls, figure, num)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "matplotlib/backends/_backend_tk.py", line 504, in create_with_canvas
    manager = cls(canvas, num, window)
              ^^^^^^^^^^^^^^^^^^^^^^^^
  File "matplotlib/backends/_backend_tk.py", line 457, in __init__
    super().__init__(canvas, num)
  File "matplotlib/backend_bases.py", line 2644, in __init__
    if rcParams['toolbar'] != 'toolmanager':
       ~~~~~~~~^^^^^^^^^^^
  File "solara/server/patch.py", line 191, in __getitem__
    return self._get_context_dict().__getitem__(key)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: 'toolbar'

[pllim]

Also, when there is a crash like above, the whole solara app grays out. I can no longer use it and have to kill the session.

[pllim]

Re: Compass crash -- It is not a problem on main using voila 0.4.5.

Also, if threading is a problem, Cubeviz movie maker might also be in trouble.

[kecnry]

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!

[kecnry]

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.

[maartenbreddels]

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.

[maartenbreddels]

https://github.com/widgetti/solara/pull/741 should fix the matplotlib issue, this was an issue on solara's side (together how the order of importing is happening).

[kecnry]

https://github.com/widgetti/solara/pull/741 should fix the matplotlib issue

Can confirm that it fixes it on my end. I'll bump the requirement of solara here once I see that is merged & released.

[kecnry]

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.

[pllim]

What about the standalone builds?

[kecnry]

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?

[pllim]

I thought Solara would be magical enough to also fix the standalone problems that were choking up voila. Perhaps I misunderstood?

[pllim]

Compass works now but...

I cannot seem to start this from Windows (not WSL2, but natively on Windows). This is not a problem on main using voila.

Traceback (most recent call last):
  File "C:\...\site-packages\reacton\core.py", line 1707, in _render
    root_element = el.component.f(*el.args, **el.kwargs)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\...\jdaviz\solara.py", line 65, in Page
    viz.load_data(data, **load_data_kwargs)
  File "C:\...\jdaviz\configs\imviz\helper.py", line 149, in load_data
    self.app.load_data(filepath, parser_reference='imviz-data-parser', **kw)
  File "C:\...\jdaviz\app.py", line 840, in load_data
    parser(self, file_obj, **kwargs)
  File "C:\...\jdaviz\configs\imviz\plugins\parsers.py", line 86, in parse_data
    file_obj = download_uri_to_path(
               ^^^^^^^^^^^^^^^^^^^^^
  File "C:\...\site-packages\jdaviz\utils.py", line 637, in download_uri_to_path
    local_path = os.path.join(os.environ["JDAVIZ_START_DIR"], local_path)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen ntpath>", line 149, in join
  File "<frozen genericpath>", line 164, in _check_arg_types
TypeError: join() argument must be str, bytes, or os.PathLike object, not 'NoneType'

[kecnry]

I cannot seem to start this from Windows (not WSL2, but natively on Windows).

Can you please help debug as I have not seen that? os.environ['JDAVIZ_START_DIR'] is still being populated here, but it seems either that or local_path is still None.

[pllim]

The path stuff... Not sure what happened, I cannot reproduce it now. 🤷‍♀️