Eliminate Imviz save pop-up dialog

[pllim]

Description

This pull request is to eliminate unnecessary pop-up dialog when viewer.save() is called. Requires bqplot bump (bqplot/bqplot#1397). Also avoids bqplot/bqplot#1396 .

Turns out I still cannot test it because the JS async call gets stuck in CI. Works when tested manually though.

Fixes #794

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 change log needed? If yes, is it added to CHANGES.rst?
• Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov]

Codecov Report

Patch coverage: 21.42% and project coverage change: -0.14 ⚠️

Comparison is base (82bebcb) 91.78% compared to head (86f1079) 91.65%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #929      +/-   ##
==========================================
- Coverage   91.78%   91.65%   -0.14%     
==========================================
  Files         147      148       +1     
  Lines       16262    16332      +70     
==========================================
+ Hits        14926    14969      +43     
- Misses       1336     1363      +27     

Impacted Files	Coverage Δ
...configs/default/plugins/export_plot/export_plot.py	48.38% <14.28%> (-7.62%)	⬇️
jdaviz/core/astrowidgets_api.py	97.56% <20.00%> (-1.20%)	⬇️
...daviz/configs/imviz/tests/test_astrowidgets_api.py	98.58% <50.00%> (ø)

... and 9 files 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.

[ojustino]

I tried to review this one but couldn't install a newer version of bqplot.

In a new conda environment, I tried pip install git+https://github.com/bqplot/bqplot.git@main and pip install git+https://github.com/bqplot/bqplot.git@96b0b (the merge commit for issue 1397) but ran into the same error: ERROR: Could not build wheels for bqplot which use PEP 517 and cannot be installed directly.

[pllim]

couldn't install a newer version of bqplot

@ojustino , have you opened an issue about the install problem over at https://github.com/bqplot/bqplot ? The devs are quite responsive.

[kecnry]

I could install bqplot 0.12.31, and can confirm the save method does not show a file dialog... but the image seems to never actually write to disk (at least not in several minutes, with the default Imviz example notebook data). It seems the asynchronous call is hanging and any future calls to save are blocked with: Exception: get_png_data already in progress.

[pllim]

It seems the asynchronous call is hanging

I see this same behavior in CI but not when I run the example notebook. So maybe this is a real bug after all.

I will remove "ready for final review" label until we have the SME back to follow-up on this.

[duytnguyendtn]

So interestingly, I installed this PR and I was able to successfully call viewer.save(), have it output an image, and no save dialog popped up. For reference pip grabbed the following bqplot versions:

bqplot==0.12.31
bqplot-image-gl==1.4.4

I would approve, but we should probably figure out the issues other users are running into

Windows 11, Python 3.9.7, Brave Browser (Chromium Backend)

[ojustino]

I was able to install bqplot 0.12.31. I went through ImvizExample.ipynb and still saw a popup asking whether to open or save the image once I reached viewer.save(). I'm on a Mac and I tested with Edge (which I know we don't support) and Firefox.

[duytnguyendtn]

As we discussed at our daily scrum, we thought maybe the error is platform dependent, so I reran this PR on my Mac and I was still able to get everything working. The only other thing I'll add is I explicitly specified the extension in my API call: viewer.save('\my\folder\test.png"). No popup, test file saved OK. Fresh environment

MacOS Mojave, Python 3.8.2, Chrome

[pllim]

It worked for me with or without extension given. The code adds the extension before deferring to JS, so that should not have mattered anyway.

I have no idea why everyone is seeing a different behavior!

[duytnguyendtn]

if user == 'ojustino': prompt_dialog()
elif user == 'kecnry': raise Exception
else: pass

:P

[kecnry]

I restarted, did a fresh install, and it worked perfectly as advertised. I then opened and ran another notebook (in parallel), reran and got the same hanging behavior I saw earlier. At that point, closing all other notebooks and restarting jupyter still didn't do the trick (I'm guessing because there is a hanging process in the background... but I can't seem to find what it might be called to kill it manually).

Upon further testing, I'm not sure I can track down a consistent set of steps to reproduce. But once it does stop working (seems to be triggered by multiple notebook instances, sometimes), it seems to keep failing (until perhaps a re-install?). Once it does stop working, restarting the kernel removes the error message on the first call to save, but there is still no file output. Subsequent calls then raise the exception.

Can anyone reproduce any of what I'm seeing?

[pllim]

If it helps, this is the relevant bqplot PR: bqplot/bqplot#1397

If this continues to be a problem, we should open a new bqplot issue.

[kecnry]

I rebased, re-milestoned, and added the same solution to the plugin API. I am no longer seeing the same issues locally that I was when this was first opened, but would be great to see if others are seeing the same things as well or not.

Snippet to test in plugin:

viz.plugins['Export Plot'].save_figure('test.png')

EDIT: I also attempted to unskip the skipped test, and although it does not hang, it does not seem to work within CI (the assertion fails). If anyone else can reproduce this locally, we can always implement a switch to force/skip the dialog.

[rosteen]

Works for me, other than one minor hiccup. When I tried to save out with cubeviz.plugins['Export Plot'].save_figure('~/Downloads/test_export.png'), the command ran with no errors but I was unable to find the file anywhere. When I specified the full path rather than using ~ it worked as expected. I don't think this is a dealbreaker, I think I remember Python not handling the ~ well in the past generally for other things either.

[rosteen]

LGTM now, thanks!

[pllim]

Yay, after 2 years. Thanks for wrapping this up, @kecnry !