Add support for getters #587

bnmajor · 2022-12-14T03:37:27Z

-- WIP --

Guided by the items discussed in #568 this branch attempts to add support for getter functions. Adapting approaches in the ipython_blocking and jupyter-ui-poll libraries the general approach is to:

Temporarily overwrite execute_request handler in IPython in order to collect and queue requests
Once the viewer is created the background thread calls execute_next_request to start the cycle of processing the remaining cells
Pop requests off one at a time and inspect for getters
- If getters are found we create a future and attach a callback that sets the result once it is received. Once all results have resolved the cell is re-run so that it may return the results.
- If no getters are found the request is just run as usual

Issues:

Currently the user namespace is not being updated when the cell is re-run properly so cell output is always wrong

review-notebook-app · 2022-12-14T03:37:31Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

thewtex · 2022-12-22T19:41:28Z

itkwidgets/viewer.py

+        if 'getImageInterpolationEnabled' not in self.results:
+            self.call_getter('getImageInterpolationEnabled')
+        elif self.results['getImageInterpolationEnabled'].done():
+            return self.results.pop('getImageInterpolationEnabled').result()


Can we put this sequence of calls in a decorator?

Yup, updated!

bnmajor · 2022-12-27T15:30:00Z

This approach is making progress but is still a bit naive and has a lot of additional cases/needs to take into account. Starting a list here of key concerns and shortcomings so that they are addressed as this moves forward:

We are currently running getters twice - once before and once during cell execution. This approach needs to be re-thought as there are plenty of use cases where this would not return the correct value. ex:

print(viewer.get_axes_enabled()) # False by default
viewer.set_axes_enabled(True)
print(viewer.get_axes_enabled()) # Should be True, will return False

Need to confirm that none of the approaches that we settle on inadvertently affect the notebook history or output
Currently using regex to identify getters. There are plenty of cases where a getter or setter may not be called directly so this is not a viable approach.

thewtex · 2022-12-27T16:11:00Z

Todos

Multiple viewers
Test notebook: 1) set, get, throws exceptions
Intercept getters during execution instead of after

which should help with all three points above.

-> merge

Create another issue as needed for setting cell output with a getter output.

thewtex · 2023-01-04T12:57:59Z

@bnmajor this timeout only occurs when the tests are executed through nbmake?

@alex-treebeard are notebook events processed in a different way when executed through nbmake?

thewtex · 2023-01-04T14:43:31Z

@alex-treebeard suggested that there may be a to resolve via the way nbmake uses nbclient.

bnmajor · 2023-01-05T21:13:35Z

@thewtex So it seems like the nbmake issue is not actually with the getters/setters but with the fact that in testing the itk-viewer is never actually created (so any cells with getters/setters timeout waiting for it to be created before being run). If I use the --overwrite flag to see the results produced from testing there is no embedded screenshot which seems to backup this theory....

thewtex · 2023-01-06T04:02:04Z

@bnmajor oh, good catch! 💡

Indeed

This widget rendering is not performed against a browser during execution

@oeway any ideas / suggestions / pointers to have ImJoy talk to a headless chromium browser?

alex-treebeard · 2023-01-18T14:47:47Z

@thewtex been doing a bit of reading around widgets. Is it necessary to load the browser in order to initialise the ITK widget?

What needs to run in order to unblock the getters from executing against an initialised viewer object?

thewtex · 2023-01-18T17:36:05Z

@alex-treebeard

Is it necessary to load the browser in order to initialise the ITK widget?

It is not necessary to import and run the Python code. However, a browser is necessary for the graphical context and JavaScript environment to perform renderings and work with the client-side widget state.

What needs to run in order to unblock the getters from executing against an initialised viewer object?

One way or another, we need to be talking to, e.g., Chrome / Electron in Headless Mode, when running in nbmake / nbclient.

alex-treebeard · 2023-01-20T18:08:19Z

@thewtex have you checked out galata?

It's possible to stand up a jupyter lab instance in a test environment, then we could potentially run nbmake against its kernel.

thewtex · 2023-01-21T04:12:07Z

@alex-treebeard great idea!!

Yes, we could use a version of nb_run.py that uses galata and something along the lines of:

https://github.com/jupyterlab/jupyterlab/blob/2250b956294558de58a99e4a26286270654eb416/galata/test/galata/notebook.spec.ts#L91-L115

Getters are designed with the expectation that they will be called twice. On the first pass the getter is added to a queue and a callback function is set on each. Once the getter resolves the result is set in the results dict and getters are ready to be called again to return their result.

This allows us to grab the cell id and raw cell string for re-running cells with getters.

Intercept execute_requests (which will be all cells queued to run after the Viewer class has been instantiated. Once the itk_viewer has been created the queue is processed one request at a time. If a cell contains getters, futures are created for each and a callback function is set to set the result as they resolve. Once all of the results are set the cell is run and the queue continues to be processed.

If we have a current request when the queue is empty we still need to execute that cell.

thewtex · 2023-11-30T17:39:36Z

Let's merge this, then, in separate PR's

Document getters limitations in sphinx
Address the JS testing issues

Improved testing is a current work in progress (as discussed in issue InsightSoftwareConsortium#587). Until we resolve the outstanding issues no viewer is actually being created, so the queue that waits for cell completion before progressing causes all subsequent cells after the first viewer is created to fail. This temporarily ignores those cells in the CI tests. These changes should be removed after testing is fixed.

bnmajor force-pushed the getters branch 2 times, most recently from c4df265 to dd42dbf Compare December 14, 2022 03:39

bnmajor force-pushed the getters branch 2 times, most recently from 9a35f4e to 42f05fa Compare December 22, 2022 19:38

thewtex reviewed Dec 22, 2022

View reviewed changes

bnmajor force-pushed the getters branch from 42f05fa to f103ed8 Compare December 27, 2022 14:48

thewtex mentioned this pull request Feb 14, 2023

Approach to block for ImJoy plugin in initialization #466

Closed

alex-treebeard mentioned this pull request Feb 16, 2023

Widget calls requiring browser context hang during testing treebeardtech/nbmake#99

Open

thewtex mentioned this pull request May 26, 2023

Feature Request: Programmatic Screenshot Export #658

Open

bnmajor added 10 commits November 30, 2023 08:42

DOC: Add getter example to Hello3DWorld notebook

9a998a0

ENH: Listen to pre_run_cell signal

e1a74b7

This allows us to grab the cell id and raw cell string for re-running cells with getters.

MAINT: Refactor getters into decorator

0f59c64

BUG: Force exceptions to bubble up and interrupt notebook

6b77217

ENH: Run the cell and then wait for getters to resolve

4138f47

MAINT: Simplify callback for getters

6b03793

ENH: Do not use background thread for setters

06aa218

ENH: Make CellWatcher class a singleton

cba9c04

bnmajor added 7 commits November 30, 2023 09:04

ENH: Use Viewers class to manage all Viewer instances

44e9913

DOC: Add more detailed comments

1915084

BUG: Abort all queued cells if an exception is thrown

e6f0bcc

ENH: Add missing getter get_layer_names

c2783ca

BUG: Fix logic for executing next request

d0c41c2

If we have a current request when the queue is empty we still need to execute that cell.

ENH: Update the list of deferred methods

916dd5c

DOC: Add notebook specifically for testing getters/setters

397fc00

bnmajor force-pushed the getters branch from a140ebf to 397fc00 Compare November 30, 2023 14:22

BUG: Add missing global variable back in

2218712

thewtex approved these changes Nov 30, 2023

View reviewed changes

bnmajor marked this pull request as ready for review November 30, 2023 17:54

BUG: Update Viewer status before call_soon

a0b8352

bnmajor force-pushed the getters branch from 34129e2 to a0b8352 Compare November 30, 2023 17:56

thewtex approved these changes Nov 30, 2023

View reviewed changes

bnmajor merged commit 3f01065 into InsightSoftwareConsortium:main Nov 30, 2023
6 checks passed

bnmajor deleted the getters branch November 30, 2023 18:39

This was referenced Dec 11, 2023

Getters improvements #702

Merged

Need to make getters available #568

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for getters #587

Add support for getters #587

bnmajor commented Dec 14, 2022

review-notebook-app bot commented Dec 14, 2022

thewtex Dec 22, 2022

bnmajor Dec 27, 2022

bnmajor commented Dec 27, 2022 •

edited by thewtex

Loading

thewtex commented Dec 27, 2022

thewtex commented Jan 4, 2023

thewtex commented Jan 4, 2023

bnmajor commented Jan 5, 2023 •

edited

Loading

thewtex commented Jan 6, 2023

alex-treebeard commented Jan 18, 2023

thewtex commented Jan 18, 2023 •

edited

Loading

alex-treebeard commented Jan 20, 2023

thewtex commented Jan 21, 2023

thewtex commented Nov 30, 2023

Add support for getters #587

Add support for getters #587

Conversation

bnmajor commented Dec 14, 2022

review-notebook-app bot commented Dec 14, 2022

thewtex Dec 22, 2022

Choose a reason for hiding this comment

bnmajor Dec 27, 2022

Choose a reason for hiding this comment

bnmajor commented Dec 27, 2022 • edited by thewtex Loading

thewtex commented Dec 27, 2022

thewtex commented Jan 4, 2023

thewtex commented Jan 4, 2023

bnmajor commented Jan 5, 2023 • edited Loading

thewtex commented Jan 6, 2023

alex-treebeard commented Jan 18, 2023

thewtex commented Jan 18, 2023 • edited Loading

alex-treebeard commented Jan 20, 2023

thewtex commented Jan 21, 2023

thewtex commented Nov 30, 2023

bnmajor commented Dec 27, 2022 •

edited by thewtex

Loading

bnmajor commented Jan 5, 2023 •

edited

Loading

thewtex commented Jan 18, 2023 •

edited

Loading