plugin active status based on pings #2295

kecnry · 2023-07-12T17:23:22Z

Description

This pull request is an alternative to #2293 and controls the live-previews (as well as keypress events and any other optimizations) based on determining whether any instance of a particular plugin is opened by having the individual instances send "I'm alive" pings back to python at a regular interval. Once these pings are no longer received, the plugin (on the python side) is determined to be inactive.

NOTE: this can result in flickering if the intervals are not tuned correctly, so it would be useful to have multiple devs/machines/data to test and make sure the interval and interval-factor are robustly set here.

Screen.Recording.2023-07-12.at.2.39.58.PM.mov

Lastly, for any plugin for which plugin.uses_active_status == True, this exposes the keep_active traitlet (and switch in the UI if it is passed to the template) as well as an as_active context manager that can be used for workflows in the API:

Screen.Recording.2023-07-12.at.2.35.56.PM.mov

This PR affects the following plugins:

line lists: redshift information updates regardless of plugin active state, this section of the code is now also optimized to use send_state (see replace all hacky traitlet replacements with send_state #2285) to avoid any unnecessary lag.
markers: the visibility of markers and the "m" keypress event are controlled by the active state of the plugin.
plot options: stretch histogram is first created whenever the plugin is first opened in the tray or via plugin.show(), and is only updated when the plugin is active.
compass: the compass image is first created whenever the plugin is first opened in the tray or via plugin.show(), and is only updated when the plugin is opened (in the tray, popup, or in-line).
line profile XY: the "l" keypress event is only enabled when the plugin is active.
line analysis: continuum preview marks are visible based on the plugin's "active" status, but the results table continues to update regardless. plugin.show_continuum_marks() which used to allow showing these marks from the API is now deprecated, in favor of using plugin.persistent_previews = True or with plugin.temporarily_show_previews():
spectral extraction: preview marks are visible based on the plugin's "active" status. Previous behavior with respect to different active sections in the plugin remains, with new visual cues.

Development considerations:

when a plugin implements preview marks or keypress events that depend on a plugin's active status, it should set self.uses_active_status = True and pass :uses_active_status="uses_active_status" @plugin-ping="plugin_ping($event)" :keep_active.sync="keep_active" to j-tray-plugin (as indicated in the developer docs). It should then observe the is_active traitlet to control the visibility of any marks.
tests should use plugin.keep_active = True or plugin.plugin_opened = True rather than plugin.open_in_tray() as the traitlet is no longer required to be immediately set.

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?
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.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

kecnry · 2023-07-12T17:28:46Z

jdaviz/app.vue

-                      <jupyter-widget :widget="trayItem.widget"></jupyter-widget>
+                      <jupyter-widget :widget="trayItem.widget" v-if="state.tray_items_open.includes(index)"></jupyter-widget>


this is needed so that the same logic works for plugins in the tray (otherwise the plugin element when collapsed will continue sending pings)

kecnry · 2023-07-12T19:30:21Z

jdaviz/components/tray_plugin.vue

+    sendPing() {
+      if (!this.$el.isConnected) {
+        return
+      }
+      this.$emit('update:plugin_ping', Date.now())
+      setTimeout(() => {
+        this.sendPing()
+      }, 200)  // ms
+    }
+  },
+  mounted() {
+    this.sendPing();
+  },


this is where any instance of the UI sends pings back to python (by updating the plugin_ping traitlet with the current timestamp) at regular intervals until it is no longer connected.

kecnry · 2023-07-12T19:32:05Z

jdaviz/core/template_mixin.py

+    @observe('plugin_ping')
+    def _plugin_ping_changed(self, *args):
+        if self.plugin_ping == 0:
+            # initial traitlet state
+            return
+        # we've received a ping, so immediately set plugin_opened state to True
+        if not self.plugin_opened:
+            self.plugin_opened = True
+
+        if self._inactive_thread is not None and self._inactive_thread.is_alive():
+            # a thread already exists to check for pings, the latest ping will allow
+            # the existing while-loop to continue
+            return
+
+        # create a thread to monitor for pings.  If a ping hasn't been received in the
+        # expected time, then plugin_opened will be set to False.
+        self._inactive_thread = threading.Thread(target=self._watch_active)
+        self._inactive_thread.start()
+
+    def _watch_active(self):
+        # expected_delay_ms hould match value in setTimeout in tray_plugin.vue
+        # NOTE: could control with a traitlet, but then would need to pass through each
+        # <j-tray-plugin> component
+        expected_delay_ms = 200
+        # plugin_ping (ms) set by setTimeout in tray_plugin.vue
+        # time.time() is in s, so need to convert to ms
+        while time.time()*1000 - self.plugin_ping < 2 * expected_delay_ms:
+            # at least one plugin has sent an "alive" ping within twice of the expected
+            # interval, wait a full (double) interval and then check again
+            time.sleep(2 * expected_delay_ms / 1000)
+
+        # "alive" ping has not been received within the expected time, consider all instances
+        # of the plugin to be closed
+        self.plugin_opened = False
+


on the python-side, the first ping sets plugin_opened = True and then starts a thread which will continually check to see if a new ping has been received in the expected interval... once a ping is failed to be received, plugin_opened is set to False and the thread terminates. If the plugin ever re-opens, a new thread is created.

codecov · 2023-07-14T19:21:15Z

Codecov Report

Patch coverage: 90.50% and project coverage change: -0.32% ⚠️

Comparison is base (a69493b) 90.94% compared to head (e8fdfdf) 90.62%.
Report is 28 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2295      +/-   ##
==========================================
- Coverage   90.94%   90.62%   -0.32%     
==========================================
  Files         152      152              
  Lines       17140    17370     +230     
==========================================
+ Hits        15588    15742     +154     
- Misses       1552     1628      +76

Files Changed	Coverage Δ
jdaviz/configs/imviz/tests/test_viewers.py	`100.00% <ø> (ø)`
jdaviz/configs/imviz/plugins/compass/compass.py	`73.21% <66.66%> (-19.89%)`	⬇️
...igs/specviz/plugins/line_analysis/line_analysis.py	`94.86% <75.75%> (-2.20%)`	⬇️
jdaviz/core/template_mixin.py	`91.48% <75.75%> (-1.52%)`	⬇️
...s/imviz/plugins/line_profile_xy/line_profile_xy.py	`97.22% <94.87%> (-1.62%)`	⬇️
jdaviz/configs/cubeviz/plugins/tests/test_tools.py	`100.00% <100.00%> (ø)`
...z/configs/default/plugins/line_lists/line_lists.py	`75.11% <100.00%> (-0.28%)`	⬇️
...efault/plugins/line_lists/tests/test_line_lists.py	`100.00% <100.00%> (ø)`
jdaviz/configs/default/plugins/markers/markers.py	`92.85% <100.00%> (-2.33%)`	⬇️
...fault/plugins/markers/tests/test_markers_plugin.py	`100.00% <100.00%> (ø)`
... and 12 more

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* mosviz.test_parsers was failing with an interpolation error on initial load

kecnry · 2023-07-17T12:07:32Z

The vast majority of uncovered lines are not possible to cover without an actual UI to send the JS pings... 🤔

kecnry · 2023-07-18T12:09:02Z

temporarily moving back to draft until addressing the following ideas/suggestions from @mariobuikhuizen offline:

use a method instead of traitlet to send the pings to avoid unnecessary overhead in syncing
have hidden/background windows/tabs considered inactivate by stop sending pings since they may be significantly throttled by the browser

* other tab, minimized window, window completely behind window, etc. * prevents "flickering" caused by browser throttling of hidden windows

kecnry · 2023-07-18T16:17:46Z

@javerbukh - made a few changes since your approval, but hopefully nothing too major/controversial 🤞

* to avoid unnecessary traitlet syncing

javerbukh · 2023-07-18T20:26:44Z

@kecnry Still works well for me!

pllim

Compass plugin appears to be broken in Imviz. (I rebased this PR on top of current main when I tested it.) I loaded images in the Imviz example notebook with JWST data and now Compass no longer shows any matplotlib image with the compass on it.

Press "l" on Line Profile in Imviz while the plugin is opened also no longer works for me. Nothing happens but there is also no error message in snackbar nor browser dev console.

Markers plugin seems to work but I kinda want the markers to still show when I hide the plugin to disable the "m to mark" feature. I feel like visibility of the existing markers should be controlled separately from this feature. Though perhaps you can also argue this is out of scope, not sure.

Out of scope but I wondered while testing this PR: Do we have plan to tie the Markers plugin to astrowidgets markers API? They don't seem to talk to each other.

jdaviz/configs/specviz/plugins/line_analysis/line_analysis.py

pllim · 2023-07-25T21:22:45Z

jdaviz/configs/specviz/plugins/line_analysis/tests/test_line_analysis.py

-    assert la_plugin.selected_line_redshift == -1.0
+    assert_allclose(la_plugin.selected_line_redshift, 0.4594414354783614)


Was this independently confirmed? I am not familiar with this workflow to confirm this myself.

jdaviz/core/template_mixin.py

pllim · 2023-07-25T21:28:59Z

jdaviz/core/template_mixin.py

+        expected_delay_ms = 200
+        # plugin_ping (ms) set by setTimeout in tray_plugin.vue
+        # time.time() is in s, so need to convert to ms
+        while time.time()*1000 - self._ping_timestamp < 2 * expected_delay_ms:


Parenthesis make it easier for me to read

Suggested change

while time.time()*1000 - self._ping_timestamp < 2 * expected_delay_ms:

while (time.time()*1000 - self._ping_timestamp) < (2 * expected_delay_ms):

Also, why repeating 2 * expected_delay_ms so many times instead of setting expected_delay_ms = 400 and be done with it?

Furthermore, why not define the expected delay in seconds to save you some unit conversion?

Because this way expected_delay_ms matches the value in javascript (as the comments state). We could have a separate variable that converts to seconds and adds the multiplicative factor, but as we tweak the delays, we might want different factors in the for loop and the sleep 🤷‍♂️

But right now they are the same, and seems like a lot of repeated calculations that are unnecessary. 🤷

kecnry · 2023-07-26T12:36:22Z

Compass plugin appears to be broken in Imviz. (I rebased this PR on top of current main when I tested it.) I loaded images in the Imviz example notebook with JWST data and now Compass no longer shows any matplotlib image with the compass on it.

Press "l" on Line Profile in Imviz while the plugin is opened also no longer works for me. Nothing happens but there is also no error message in snackbar nor browser dev console.

This is concerning - are you on windows? Can someone else with windows confirm (@duytnguyendtn)?

pllim · 2023-07-26T14:34:20Z

are you on windows?

WSL2 (Debian) on Windows 10. But the browser is Chrome on Windows. They have worked before.

by not passing to template, this then means all plugins should listen to is_active and never plugin_opened

kecnry · 2023-07-27T16:24:21Z

tested on the platform and didn't seem to have any issues with respect to network lag causing flickering (except the popout windows fail in general, but that's unrelated to this PR)

@pllim - compass and line profile have been updated to use is_active directly now and should work. Please re-test with your use-cases though!

pllim

Compass and Line Profile both work now on Imviz. Thanks!

This reverts commit 2e7f386.

kecnry added UI/UX😍 plugin Label for plugins common to multiple configurations labels Jul 12, 2023

kecnry added this to the 3.6 milestone Jul 12, 2023

github-actions bot added documentation Explanation of code and concepts cubeviz specviz embed Regarding issues with front-end embedding imviz specviz2d labels Jul 12, 2023

kecnry commented Jul 12, 2023

View reviewed changes

kecnry force-pushed the plugin-ping branch 5 times, most recently from 74df202 to b5f52ad Compare July 12, 2023 19:02

kecnry commented Jul 12, 2023

View reviewed changes

kecnry mentioned this pull request Jul 12, 2023

live-preview marks based on mouseover #2293

Closed

9 tasks

kecnry force-pushed the plugin-ping branch from eaf0632 to a0a7635 Compare July 14, 2023 17:47

kecnry added 4 commits July 14, 2023 15:26

plugin active status determined by "alive" pings

09cf615

code cleanup

777a6e0

fix broken tests

c8e25e8

don't crash if line analysis fails in specutils

4e0bdcd

* mosviz.test_parsers was failing with an interpolation error on initial load

kecnry force-pushed the plugin-ping branch from 99f76af to b6401d8 Compare July 14, 2023 19:26

use plugin_opened = True in tests instead of open_in_tray()

c25044d

kecnry force-pushed the plugin-ping branch from b6401d8 to c25044d Compare July 17, 2023 12:06

kecnry marked this pull request as ready for review July 17, 2023 12:07

kecnry requested review from duytnguyendtn and rosteen as code owners July 17, 2023 12:07

kecnry added the Ready for final review label Jul 17, 2023

kecnry marked this pull request as draft July 18, 2023 12:09

consider "hidden" plugin to be inactive

0443bbf

* other tab, minimized window, window completely behind window, etc. * prevents "flickering" caused by browser throttling of hidden windows

replace ping plugin with method

39098f0

* to avoid unnecessary traitlet syncing

kecnry force-pushed the plugin-ping branch from 8b280bd to 39098f0 Compare July 18, 2023 16:18

kecnry marked this pull request as ready for review July 18, 2023 17:10

pllim requested changes Jul 25, 2023

View reviewed changes

separate changelog entry for API deprecation

68c52df

kecnry added 4 commits July 26, 2023 12:00

allow skipping keep_active UI

d69a8ca

by not passing to template, this then means all plugins should listen to is_active and never plugin_opened

update compass to use is_active

f29b0ee

update line_profile to use is_active

2158220

update plot options to use is_active

39f5b4b

kecnry force-pushed the plugin-ping branch from 0ee1533 to 39f5b4b Compare July 26, 2023 16:58

kecnry requested a review from pllim July 27, 2023 16:24

more verbose test for line assignment

e8fdfdf

pllim approved these changes Jul 27, 2023

View reviewed changes

pllim merged commit 2e7f386 into spacetelescope:main Jul 27, 2023
11 of 14 checks passed

kecnry deleted the plugin-ping branch July 27, 2023 21:39

pllim mentioned this pull request Jul 28, 2023

Imviz unusable in Windows using Jdaviz 3.6 or later #2317

Closed

pllim added a commit that referenced this pull request Jul 28, 2023

Revert "plugin active status based on pings (#2295)"

2b3d4b5

This reverts commit 2e7f386.

pllim mentioned this pull request Jul 28, 2023

Revert "plugin active status based on pings" #2318

Closed

This was referenced Jul 31, 2023

update is_active implementation from jdaviz 3.6 release spacetelescope/lcviz#33

Merged

Fix ping lag in plot options #2326

Merged

kecnry mentioned this pull request Sep 12, 2023

improvements to is_active ping logic #2450

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugin active status based on pings #2295

plugin active status based on pings #2295

kecnry commented Jul 12, 2023 •

edited

Loading

kecnry Jul 12, 2023

kecnry Jul 12, 2023

kecnry Jul 12, 2023

codecov bot commented Jul 14, 2023 •

edited

Loading

kecnry commented Jul 17, 2023

kecnry commented Jul 18, 2023 •

edited

Loading

kecnry commented Jul 18, 2023 •

edited

Loading

javerbukh commented Jul 18, 2023

pllim left a comment

pllim Jul 25, 2023

pllim Jul 25, 2023

kecnry Jul 26, 2023

pllim Jul 26, 2023

kecnry commented Jul 26, 2023

pllim commented Jul 26, 2023

kecnry commented Jul 27, 2023

pllim left a comment

		<jupyter-widget :widget="trayItem.widget"></jupyter-widget>
		<jupyter-widget :widget="trayItem.widget" v-if="state.tray_items_open.includes(index)"></jupyter-widget>

		assert la_plugin.selected_line_redshift == -1.0
		assert_allclose(la_plugin.selected_line_redshift, 0.4594414354783614)

	while time.time()1000 - self._ping_timestamp < 2 expected_delay_ms:
	while (time.time()1000 - self._ping_timestamp) < (2 expected_delay_ms):

plugin active status based on pings #2295

plugin active status based on pings #2295

Conversation

kecnry commented Jul 12, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

kecnry Jul 12, 2023

Choose a reason for hiding this comment

kecnry Jul 12, 2023

Choose a reason for hiding this comment

kecnry Jul 12, 2023

Choose a reason for hiding this comment

codecov bot commented Jul 14, 2023 • edited Loading

Codecov Report

kecnry commented Jul 17, 2023

kecnry commented Jul 18, 2023 • edited Loading

kecnry commented Jul 18, 2023 • edited Loading

javerbukh commented Jul 18, 2023

pllim left a comment

Choose a reason for hiding this comment

pllim Jul 25, 2023

Choose a reason for hiding this comment

pllim Jul 25, 2023

Choose a reason for hiding this comment

kecnry Jul 26, 2023

Choose a reason for hiding this comment

pllim Jul 26, 2023

Choose a reason for hiding this comment

kecnry commented Jul 26, 2023

pllim commented Jul 26, 2023

kecnry commented Jul 27, 2023

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Jul 12, 2023 •

edited

Loading

codecov bot commented Jul 14, 2023 •

edited

Loading

kecnry commented Jul 18, 2023 •

edited

Loading

kecnry commented Jul 18, 2023 •

edited

Loading