plugin is_active skip_if_no_updates_since_plugin_last_active decorator

[kecnry]

Description

This pull request implements (and uses) a @skip_if_no_updates_since_plugin_last_active decorator for plugin methods that observe is_active (among other traitlets) to avoid queuing of expensive calls that can result in laggy behavior when browser throttle the pings resulting in constant toggling of is_active. This is essentially a more generic implementation of the same principal as #2326 and then applied to all plugins which currently use is_active.

Currently this only needs to be updated from the flattening plugin in lcviz: spacetelescope/lcviz#38, and eventually the binning plugin (unmerged).

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)?

[codecov]

Codecov Report

Patch coverage: 93.82% and project coverage change: -0.10% ⚠️

Comparison is base (90ddefb) 90.61% compared to head (3af058f) 90.52%.
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2386      +/-   ##
==========================================
- Coverage   90.61%   90.52%   -0.10%     
==========================================
  Files         159      159              
  Lines       18177    18215      +38     
==========================================
+ Hits        16471    16489      +18     
- Misses       1706     1726      +20     

Files Changed	Coverage Δ
...viz/configs/imviz/plugins/footprints/footprints.py	96.33% <ø> (ø)
...plugins/spectral_extraction/spectral_extraction.py	90.76% <91.48%> (-0.59%)	⬇️
jdaviz/core/template_mixin.py	90.42% <94.73%> (+0.05%)	⬆️
...nfigs/default/plugins/plot_options/plot_options.py	93.62% <100.00%> (+0.26%)	⬆️
jdaviz/configs/imviz/plugins/compass/compass.py	67.79% <100.00%> (-2.90%)	⬇️
...s/imviz/plugins/line_profile_xy/line_profile_xy.py	97.22% <100.00%> (ø)
...igs/specviz/plugins/line_analysis/line_analysis.py	91.27% <100.00%> (-3.71%)	⬇️

... and 4 files with indirect coverage changes

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

[pllim]

Should this append happen before meth(...) is invoked, in case it is an intensive method that takes a long time?

[kecnry]

that's a good idea.... my only concern is what happens if there is an exception in meth(...). But maybe we could wrap meth(...) in a try/except to remove from the list if it fails (and then raise the original error)? I'll think if there would be any unintended consequences of that, but I can see how otherwise this could still technically result in a (small, but unnecessary) queue of calls.

[pllim]

So it didn't crash but I did find this using the Specviz2D example notebook. Load the data, open the Spectral Extraction plugin, then run this:

plg = specviz2d.plugins["Spectral Extraction"]
for i in range(5):
    plg.bg_width = i

When it completes, the GUI shows width at 4, which is correct. But the markers disappear. They come back if you manually adjust the width to 5 and back to 4.

Not sure if this is something we have to worry about but it is technically public API.

[pllim]

I thought I picked up your commit 1508970 but rebased on latest main, I still see the same problem. Windows 10 + Chrome

[kecnry]

it seemed to have fixed the problem on my end, but let me try to see if I can find another way to reproduce (we may need something like your suggestion above after all).

[pllim]

I dunno if any small lag is greatly magnified on Windows or something. If you or another dev have access to Windows machine, please try it out there. I want to make sure it is not just me.

[pllim]

But if this is OS specific, maybe we can open up a follow up issue and defer it. It is uncommon for someone to set such parameters non-stop in a loop, right?

[kecnry]

@pllim - I think (hope) this should be much more reliable now. I really went through and refactored quite a few of the methods in spectral extraction so only the expensive extracting step follows this decorator, so that the other "active step" stuff is not affected.

[rosteen]

I didn't manage to break anything, tested all the plugins that this touches. I think this is good to go in. I think I even mostly understand what it's doing 😅

[pllim]

Let me try one more time on Windows, but I need to review the Footprint one first.

[pllim]

The previous problem is gone for me. Thanks!