[MRG] Update GUI to latest version of ipywidgets and voila #696

gtdang · 2023-12-08T16:50:49Z

Completed

Removed pinning of older versions ipywidgets, ipympl, and voila. Set a requirement for ipywidgets >=8.1.1, because of breaking changes to v8.x.
- Updating to the latest versions resolved js errors that were previously preventing the GUI from rendering, which was the impetus for pinning versions hnn-gui won't launch due to javascript error after clean install #585 [MRG] MAINT: Restrict voila version #586.
Addressed some ipywidgets 8.x API changes that prevented GUI composition, running simulations, and plotting.
- Implemented new methods of assigning titles and children to collection widgets (Tabs and Accordion). ~~Though only to the scope of what was causes crashes with what I was testing (composition and simulation launch).~~ There could be more areas were it should be updated.
- Linking of Tab collections was causinging an issue with new ipywidgets validation logic. Solution was to write a decorator function that unlinks linked Tab collections (Figure tabs and Fig-Config tab groups) before editing functions and re-links them upon completion.
  - Added test for decorator.
- Updated to new FileUpload widget API
  - This involved restructuring the input structure to the widget. It now takes a list of dicts instead of a single dict.
  - Access to the file upload dict items was updated.
  - For the file upload pytests the simulated file dict also had to be restructured to the new dictionary structure FileUpload expects.
  - Removed setting of the deprecated _counter trait of FileUpload.
- Added ipykernel as a GUI dependency. ipywidgets removed its dependency of ipykernel to allow more flexibility to use other kernels. This might only need to be a test dependency?

Remarks

I've only tested the using the GUI to launch the default simulation and adding some of the plots. Will need to test/search for more updates to the ipywidgets 8.x API. I suspect features like file-upload also need to be updated.

Questions

The FileUpload button widget has a counter displayed showing the number of files uploaded. This counter cannot be disabled. (FileUpload displayed counter should be optional jupyter-widgets/ipywidgets#2904) For the network connectivity and drives upload button, the current code always keeps this number at 0. Do we want to keep it that way or should it counter increment to 1 if the user loads a param file?
- Personally I don't think the counter is great for our application and wish it could be removed... I think the user does need to have some confirmation that a file has been loaded though. An informational display of what files have been loaded for network and drive would be nice. And this could even show that a default file has been loaded at the start. We decided to keep the counter always at 0.
- On a slightly related note, this widget is able to accept multiple files. Would this be useful for loading multiple data files? That could be a feature we can add in a future PR.

To do:

Check file upload feature
Test for other functionality that may error
Add unit test for decorator

…gnment

… objects when adding figures. This is to function with ipywidgets 8.* implementation of selection index change validation for Tab objects.

…d removed version pins of voila and ipympl.

…lib 8.x deprecation of ax._get_lines.prop_cycler.

…gnment

… objects when adding figures. This is to function with ipywidgets 8.* implementation of selection index change validation for Tab objects.

…d removed version pins of voila and ipympl.

…latest-ipywidgets-version

ntolley · 2023-12-08T18:47:12Z

hnn_core/gui/_viz_manager.py

@@ -711,6 +739,7 @@ def compose(self):
        ])
        return config_panel, fig_output_container

+    @unlink_relink(attribute='figs_config_tab_link')


Need to think of one line comment on ipywidgets figure linking logic

Perhaps: "Unlinks Tab objects to avoid index error from asynchronous addition of new tabs."

sorry I missed the discussion on this, is it a bugfix or something related to the new version of ipywidgets?

gtdang · 2023-12-11T16:59:25Z

Just documenting the failed tests for areas that need to be addressed yet.

FAILED hnn_core/tests/test_gui.py::test_gui_upload_params - traitlets.traitlets.TraitError: The 'value' trait of a FileUpload instance expected a tuple, not the dict...
FAILED hnn_core/tests/test_gui.py::test_gui_upload_data - traitlets.traitlets.TraitError: The 'value' trait of a FileUpload instance expected a tuple, not the dict...
FAILED hnn_core/tests/test_gui.py::test_gui_take_screenshots - AttributeError: 'Tab' object has no attribute '_titles'
FAILED hnn_core/tests/test_gui.py::test_gui_adaptive_spectrogram - AttributeError: 'Tab' object has no attribute '_titles'

This is pretty consistent with ipywidgets 8.x API changes to FileUpload and accessing collection widget attributes. Good to know!

…lified logic.

…ignment for collections. Changed del in favor of the pop method to remove from lists and dicts because it was causing an error. Added comments.

… widget. Updated "value" trait from a dict to list.

codecov-commenter · 2023-12-13T17:26:42Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (1216038) 91.34% compared to head (2d4bd76) 91.35%.
Report is 7 commits behind head on master.

❗ Current head 2d4bd76 differs from pull request most recent head 96e4a6a. Consider uploading reports for the commit 96e4a6a to get more accurate results

Files	Patch %	Lines
hnn_core/gui/_viz_manager.py	83.87%	5 Missing ⚠️
hnn_core/gui/gui.py	92.30%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
+ Coverage   91.34%   91.35%   +0.01%     
==========================================
  Files          25       25              
  Lines        4599     4606       +7     
==========================================
+ Hits         4201     4208       +7     
  Misses        398      398

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

gtdang · 2023-12-14T18:48:24Z

@ntolley Seems like the macOS tests are passing now. It's failing on docs and link check? Are you familiar with these errors?

ntolley · 2023-12-14T21:45:14Z

New error for me as well @gtdang

Looks like it's a weird consequence of one of the version changes with the ipywidgets?

ntolley · 2023-12-15T16:59:37Z

@gtdang I did some digging and I think the answer is to add ipykernel as an explicit dependency:

https://stackoverflow.com/questions/76855161/no-such-kernel-named-python3-ipywidgets-ipykernel-dependency-issue

jupyter-widgets/ipywidgets#3731

…is as dependency so we must explicitly include for CI tests.

gtdang · 2023-12-18T20:34:14Z

@ntolley adding ipykernel as a GUI dependency seemed to do the trick!

Now the docs build is timing out as it's taking longer than 10mins. Does this tend to happen? If so, we could try increasing the timeout limit from the default 10m.

gtdang · 2023-12-18T21:19:23Z

@ntolley adding ipykernel as a GUI dependency seemed to do the trick!

Now the docs build is timing out as it's taking longer than 10mins. Does this tend to happen? If so, we could try increasing the timeout limit from the default 10m.

Hm never mind. It seems like the GUI captures are not rendering for creating the docs. I'll have to dig into why this is happening with the updated dependencies.

…ge to 20min.

gtdang · 2023-12-19T17:22:40Z

@ntolley I updated the circleci config file to bump the build timeout to 20mins. It passed this time however looking the logs the build took <2min... So I'm thinking that change wasn't necessary and the timeout was just some inconsistency/stability issue on the vm. But I guess it doesn't hurt to keep it at 20mins? I'm not sure what plan you are on and if runtime is limited.

hnn_core/tests/test_gui.py

…uild stage to 20min." This reverts commit c882122.

jasmainak · 2023-12-21T21:42:23Z

hnn_core/gui/_viz_manager.py

@@ -99,6 +99,35 @@ def plot_type_coupled_change(new_plot_type, target_data_selection):
        target_data_selection.disabled = False


+def unlink_relink(attribute: str):
+    """
+    Wrapper function to unlink widgets to perform edits and re-link them on


Check PEP 257 convention ... need a one-line description first ... longer description may follow later.

"Wrapper function" as in decorator, right?

jasmainak · 2023-12-21T21:43:17Z

hnn_core/gui/_viz_manager.py

@@ -99,6 +99,35 @@ def plot_type_coupled_change(new_plot_type, target_data_selection):
        target_data_selection.disabled = False


+def unlink_relink(attribute: str):


we don't do type annotations ... I think it will have to be a repo-wide change, let's stick to repo conventions for now to keep things consistent

jasmainak · 2023-12-21T21:45:55Z

hnn_core/gui/_viz_manager.py

@@ -565,8 +600,8 @@ def _add_figure(b, widgets, data, scale=0.95, dpi=96):
        with widgets['figs_output']:
            display(widgets['figs_tabs'])

-    widgets['figs_tabs'].children = widgets['figs_tabs'].children + (
-        fig_outputs, )
+    widgets['figs_tabs'].children = \


backslashes are not preferred ... would suggest using paranthesis if possible

jasmainak · 2023-12-21T21:48:03Z

hnn_core/tests/test_gui.py

 from hnn_core import Dipole, Network, Params
 from hnn_core.gui import HNNGUI
-from hnn_core.gui._viz_manager import _idx2figname, _no_overlay_plot_types
+from hnn_core.gui._viz_manager import _idx2figname, _no_overlay_plot_types, \


paranthesis instead of backslash

jasmainak · 2023-12-21T21:52:13Z

hnn_core/tests/test_gui.py

+def test_unlink_relink_widget():
+    """Tests the unlinking and relinking of widgets decorator."""
+
+    # Create a basic version of the VizManager class


why not use VizManager class directly for testing? what's the bottleneck?

It's mostly out of principle to make the test small and clear so that the scope of the test is just the functionality of the decorator, which makes it easier to detect if the a bug is coming from within the function vs externally.

The full functionality tests of the VizManager liketest_gui_add_figure would already catch/debug bugs that occur in VizManager and its use of the decorator.

jasmainak · 2023-12-21T21:57:18Z

But I guess it doesn't hurt to keep it at 20mins?

we need to find a solution to this ... @gtdang if you are willing to help that would be great. IMO, the GUI docs should be run separately. Currently running make html-noplot also runs the GUI examples ... it's a bug

we need a dedicated machine for running the GUI docs because they are slow ... either on a paid plan on CircleCI or through the Brown infrastructure. Not sure how we'd link it to pull requests if we do it on the Brown infrastructure though.

jasmainak · 2023-12-21T22:05:36Z

But I guess it doesn't hurt to keep it at 20mins?

Let's merge this PR with the 10 min limit and try to find a long-term solution in separate pull requests

gtdang · 2023-12-22T17:30:18Z

But I guess it doesn't hurt to keep it at 20mins?

Let's merge this PR with the 10 min limit and try to find a long-term solution in separate pull requests

We actually found another issue on Wednesday with the .capture() method no longer rendering the GUI--an issue for the GUI docs/tutorials. After some digging it's related to jupyter-widgets/ipywidgets#3871. So we'll need to find an alternative solution for capturing the state of the GUI and displaying it. Should we hold off on merging until that is figured out?

jasmainak · 2023-12-22T20:47:31Z

ughgh ... any workarounds for now?

maybe: https://html2canvas.hertzen.com/ ?

ntolley · 2023-12-28T09:36:27Z

Not a bad option @jasmainak !

Truthfully I think it may be best to keep this PR focused on making the GUI functional, and in a separate one either fix the docs or test a new solution all together

gtdang · 2024-01-08T20:54:43Z

Not a bad option @jasmainak !

Truthfully I think it may be best to keep this PR focused on making the GUI functional, and in a separate one either fix the docs or test a new solution all together

I agree. I think the docs solution will be in a separate PR. I'm not in love the the current implementation of the GUI tutorials (even when it is working properly). It's a bit confusing and resource intensive for for the end user. I think screenshots would more clear for the user, although it has the tradeoff being less a streamlined process for writers. There are ways to automate screen captures pyautogui. We can discuss more when we meet this week.

jasmainak · 2024-01-09T16:38:18Z

will pyautogui require xvfb or a virtual screen to make it work in the CIs? My concern is that exact coordinates for the CI may be difficult to guess ... but I'm open to anything that works and is a stable solution

gtdang · 2024-01-09T19:51:23Z

will pyautogui require xvfb or a virtual screen to make it work in the CIs? My concern is that exact coordinates for the CI may be difficult to guess ... but I'm open to anything that works and is a stable solution

Yes it would be a 2 step process of first running the simulation and grabbing the screenshots and then creating the documentation using the images. It shouldn't need to be done every time we regenerate docs though, only if the UI changes. You can actually give it images of buttons/tabs for it to find the coordinates to click. The images should also have the mouse cursor to highlight context.

For html2canvas... I would be against adding a js runtime dependency just for screen caps. Also the execution would be pretty difficult because you would have to invoke the js after each cell execution... not at the end of the full notebook execution.

ntolley · 2024-01-10T18:24:28Z

@jasmainak I'm good with going ahead and merging this PR as I think in the long term we are going to move to a screenshot method for tutorials (either automated or manual). In any case I think the purpose of this PR is achieved, the next one to work on is #651

Merge button is yours if you're good with its current state!

jasmainak · 2024-01-10T19:52:21Z

setup.py

@@ -106,7 +106,7 @@ def run(self):
              'h5io'
          ],
          extras_require={
-              'gui': ['ipywidgets <=7.7.1', 'ipympl<0.9', 'voila<=0.3.6'],
+              'gui': ['ipywidgets>=8.0.0', 'ipykernel', 'ipympl', 'voila'],


I think you'll need to update README:

https://jonescompneurolab.github.io/hnn-core/stable/index.html

let me know once you fix it and I'll go ahead and merge @gtdang !

Thanks! Updated!

jasmainak · 2024-01-10T22:26:00Z

Thanks @gtdang ! Congratulations on your first merged PR 🥳 🍺 🎉

gtdang added 10 commits December 4, 2023 16:03

fix: Updated accordion widget title assignment to ipywidgets 8.0 assi…

be01e85

…gnment

fix: Updated Tab children assignment to list-based assignment.

266e789

fix: Updated logic to unlink and re-link figure and figure config Tab…

bf70536

… objects when adding figures. This is to function with ipywidgets 8.* implementation of selection index change validation for Tab objects.

chore: Updated ipywidgets version dependency to greater than 8.1.1 an…

c10b708

…d removed version pins of voila and ipympl.

fix: GUI dipole plot bug. Refactored GUI color cycling to for matplot…

d3ec97e

…lib 8.x deprecation of ax._get_lines.prop_cycler.

fix: Updated accordion widget title assignment to ipywidgets 8.0 assi…

a7786ff

…gnment

fix: Updated Tab children assignment to list-based assignment.

e81d247

fix: Updated logic to unlink and re-link figure and figure config Tab…

94820fa

… objects when adding figures. This is to function with ipywidgets 8.* implementation of selection index change validation for Tab objects.

chore: Updated ipywidgets version dependency to greater than 8.1.1 an…

7ce1404

…d removed version pins of voila and ipympl.

Merge remote-tracking branch 'origin/latest-ipywidgets-version' into …

b7d45e9

…latest-ipywidgets-version

gtdang marked this pull request as draft December 8, 2023 16:50

ntolley reviewed Dec 8, 2023

View reviewed changes

fix: Indentation

bf3580c

gtdang added 6 commits December 11, 2023 16:36

fix: Refactored for ipywidgets 8.x title access for collections. Simp…

0c0e877

…lified logic.

fix: Refactored for ipywidgets 8.x title access for collections.

a629787

fix: Refactored _close_figure for ipywidgets 8.x title access and ass…

4391b46

…ignment for collections. Changed del in favor of the pop method to remove from lists and dicts because it was causing an error. Added comments.

fix: Updated FileUpload widget to ipywidgets 8.x API

94576d9

fix: removed the setting of depricated "_counter" trait of FileUpload…

a8e9ebb

… widget. Updated "value" trait from a dict to list.

refactor: line lengths

c3a805b

chore: change ipywidget version specification to ipywidgets>=8.0.0

261a025

fix: added ipykernel as a GUI dependency. ipywidgets no longer has th…

ecd2e0e

…is as dependency so we must explicitly include for CI tests.

fix: increase circleci no output timeout limit for the docs build sta…

c882122

…ge to 20min.

ntolley reviewed Dec 20, 2023

View reviewed changes

hnn_core/tests/test_gui.py Outdated Show resolved Hide resolved

gtdang added 4 commits December 20, 2023 09:33

Revert "fix: increase circleci no output timeout limit for the docs b…

01c758a

…uild stage to 20min." This reverts commit c882122.

test: refactored unlink_relink decorator

1dd4568

fix: refactored setup.py extras. Added test, docs, and dev groups.

b54b656

fix: added back ipympl and voila dependencies

e14da5f

jasmainak reviewed Dec 21, 2023

View reviewed changes

gtdang added 3 commits December 22, 2023 12:13

chore: formatting, comment clarification

2c38ba3

chore: formatting, docstring clarification, remove type annotations

f91cbb2

chore: formatting, docstring clarification, remove type annotations

96e4a6a

chore: formatting

d49cad3

gtdang changed the title ~~[WIP] Update GUI to latest version of ipywidgets and voila~~ [MRG] Update GUI to latest version of ipywidgets and voila Jan 10, 2024

ntolley approved these changes Jan 10, 2024

View reviewed changes

jasmainak reviewed Jan 10, 2024

View reviewed changes

docs: update readme

207de0c

jasmainak merged commit d3dd255 into jonescompneurolab:master Jan 10, 2024
9 checks passed

ntolley mentioned this pull request Jan 12, 2024

[MRG] Dev extras #703

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Update GUI to latest version of ipywidgets and voila #696

[MRG] Update GUI to latest version of ipywidgets and voila #696

gtdang commented Dec 8, 2023 •

edited

Loading

ntolley Dec 8, 2023

gtdang Dec 8, 2023 •

edited

Loading

jasmainak Dec 21, 2023

gtdang commented Dec 11, 2023

codecov-commenter commented Dec 13, 2023 •

edited

Loading

gtdang commented Dec 14, 2023

ntolley commented Dec 14, 2023

ntolley commented Dec 15, 2023

gtdang commented Dec 18, 2023

gtdang commented Dec 18, 2023

gtdang commented Dec 19, 2023

jasmainak Dec 21, 2023

jasmainak Dec 21, 2023

jasmainak Dec 21, 2023

jasmainak Dec 21, 2023

jasmainak Dec 21, 2023

jasmainak Dec 21, 2023

gtdang Dec 22, 2023

jasmainak commented Dec 21, 2023

jasmainak commented Dec 21, 2023 •

edited

Loading

gtdang commented Dec 22, 2023

jasmainak commented Dec 22, 2023

ntolley commented Dec 28, 2023

gtdang commented Jan 8, 2024 •

edited

Loading

jasmainak commented Jan 9, 2024

gtdang commented Jan 9, 2024

ntolley commented Jan 10, 2024

jasmainak Jan 10, 2024

gtdang Jan 10, 2024

jasmainak commented Jan 10, 2024

		@@ -99,6 +99,35 @@ def plot_type_coupled_change(new_plot_type, target_data_selection):
		target_data_selection.disabled = False


		def unlink_relink(attribute: str):

[MRG] Update GUI to latest version of ipywidgets and voila #696

[MRG] Update GUI to latest version of ipywidgets and voila #696

Conversation

gtdang commented Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

gtdang Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtdang commented Dec 11, 2023

codecov-commenter commented Dec 13, 2023 • edited Loading

Codecov Report

gtdang commented Dec 14, 2023

ntolley commented Dec 14, 2023

ntolley commented Dec 15, 2023

gtdang commented Dec 18, 2023

gtdang commented Dec 18, 2023

gtdang commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Dec 21, 2023

jasmainak commented Dec 21, 2023 • edited Loading

gtdang commented Dec 22, 2023

jasmainak commented Dec 22, 2023

ntolley commented Dec 28, 2023

gtdang commented Jan 8, 2024 • edited Loading

jasmainak commented Jan 9, 2024

gtdang commented Jan 9, 2024

ntolley commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Jan 10, 2024

gtdang commented Dec 8, 2023 •

edited

Loading

gtdang Dec 8, 2023 •

edited

Loading

codecov-commenter commented Dec 13, 2023 •

edited

Loading

jasmainak commented Dec 21, 2023 •

edited

Loading

gtdang commented Jan 8, 2024 •

edited

Loading