Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add callback mechanism for GUI mode #301

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add callback mechanism for GUI mode #301

wants to merge 10 commits into from

Conversation

elevans
Copy link
Member

@elevans elevans commented Mar 4, 2024

Update

Update I wanted to preserve the original text (below) for this PR as its relevant to the goals (i.e. create a better experience for macOS users). With the awesome work that @ctrueden has done with jaunch we now have a way to actually support interactive mode on macOS. This PR introduces the following new elements and also paves the way for interactive mode on macOS via jaunch.

This PR adds:

  • A "global gateway" which enables other projects to hook into a already running instance of imagej (e.g. napari-imagej).
  • A callback mechanism which enables users to register functions with when_imagej_starts(). This can enable some GUI related workflows on macOS (a kind of limited psuedo-interactive mode). See notebook 5 section 5.2 for the relevant documentation. This callback mechanism is not compatible with napari.
  • An override flag to the main init method. This flag will enable users/developers to bypass initialization checks/blocks we've put in place to stop unsupported/application breaking behavior -- mainly interactive mode on macOS. To test interactive mode on macOS:
    • Install/setup jaunch.
    • Modify jaunch's fiji.py file. Add the override=True flag to ij = imagej.init(app_dir, mode="interactive") on line 72.
    • Jaunch Fiji with ./fiji-macos-universal -i --python.

Some other relevant information: Based on what I now understand about Python's threading system and some experimentation on macOS/Linux with jaunch, Python will always think its in a main thread. There is no way (from what I gather) to know if a particular Python instance was started with/in a pthread. Instead, threads that are spawned from an already running instance, say in a function that creates threads, are identified as "non-main" thread. This is obvious, but it would have been cool if there was a way to know about how the current instance was started.

Original text

Until we can find a true interactive mode for macOS (see #298) this callback mechanism gives macOS users the ability to run their desired Python functions before the REPL is locked by the AppHelper.runConsoleEventLoop().

Here's how you register a callback with PyImageJ:

import imagej

# register a function that takes no params
imagej.when_imagej_starts(lambda: func())

# register an ImageJ function (if a parameter is present, assume its ij)
imagej.when_imagej_starts(lambda ij: ij.RoiManager.getRoiManager())

# initialize ImageJ in GUI mode
ij = imagej.init(mode='gui')

Unfortunately this callback strategy doesn't work with napari. Registering the callback lambda: napari.Viewer() results in this segfault:

(pyimagej) loci@dyn-144-92-48-223 Documents % python test.py
WARNING: package sun.awt.X11 not in java.desktop
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by net.imagej.legacy.IJ1Helper (file:/Users/loci/.jgo/net.imagej/imagej/RELEASE/28b74e6959b0634f2a8e10c27afe06e7039fb4a0dc582d9746d028ba40e5bd04/imagej-legacy-1.2.1.jar) to method com.apple.eawt.Application.getApplication()
WARNING: Please consider reporting this to the maintainers of net.imagej.legacy.IJ1Helper
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGILL (0x4) at pc=0x00007ff8116c90c2, pid=2487, tid=42247
#
# JRE version: OpenJDK Runtime Environment Zulu11.70+15-CA (11.0.22+7) (build 11.0.22+7-LTS)
# Java VM: OpenJDK 64-Bit Server VM Zulu11.70+15-CA (11.0.22+7-LTS, mixed mode, tiered, compressed oops, g1 gc, bsd-amd64)
# Problematic frame:
# C  [libdispatch.dylib+0x50c2]  _dispatch_assert_queue_fail+0x66
#
# No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# An error report file with more information is saved as:
# /Users/loci/Documents/hs_err_pid2487.log
#
# If you would like to submit a bug report, please visit:
#   http://www.azul.com/support/
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#
zsh: abort      python test.py
(pyimagej) loci@dyn-144-92-48-223 Documents % 

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 78.39506% with 35 lines in your changes missing coverage. Please review.

Project coverage is 77.48%. Comparing base (a173d5a) to head (a833767).
Report is 14 commits behind head on main.

Files Patch % Lines
src/imagej/__init__.py 43.33% 17 Missing ⚠️
tests/test_legacy.py 68.57% 11 Missing ⚠️
tests/test_fiji.py 12.50% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
- Coverage   77.81%   77.48%   -0.33%     
==========================================
  Files          16       17       +1     
  Lines        2010     2034      +24     
==========================================
+ Hits         1564     1576      +12     
- Misses        446      458      +12     

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

@elevans elevans force-pushed the global-gateway branch 3 times, most recently from 15e8366 to dbb930c Compare May 10, 2024 16:32
@elevans elevans force-pushed the global-gateway branch 3 times, most recently from 884c7df to 3f0188b Compare May 28, 2024 02:13
@elevans
Copy link
Member Author

elevans commented Jun 10, 2024

Just fyi all PyImageJ builds will fail for python 3.12 until labeling=0.1.14 no longer pins pillow <10 in the conda-feedstock recipe. See this issue.

Edit: This is no longer an issue with labeling 0.1.14's release.

Copy link
Contributor

@gselzer gselzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I'm excited about these changes @elevans!

I'm a bit wary about that new override parameter on the initialization function, though - let's discuss below.

src/imagej/__init__.py Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
src/imagej/__init__.py Outdated Show resolved Hide resolved
ctrueden and others added 8 commits July 15, 2024 10:17
Otherwise, there is no way to access it from other threads.

This may be useful for scenarios like napari-imagej, where access to
Python scripting is available from threads other than the blocked one.
This is an internal variable, not intended as public API.
It doesn't need to be "ij_wrapper" or "ij_fixture".
It's OK to be ij!
This test checks that functions registered via
when_imagej_starts get called as part of imagej.init.
This commit adds section 5.2 to the "Convenience methods of
PyImageJ" that describes how to use the when_image_starts
callback mechanism.
This commit introduces a check to determine if the current
running thread is the main thread. If True, then we continue to block
interactive mode on macOS (it is not possible to share the main thread
with GUI event loop needed for the ImageJ GUI). If False, then the
current thread is *not* the main thread (e.g. a jaunched session) and
interactive mode for macOS can proceed.
@elevans elevans force-pushed the global-gateway branch 2 times, most recently from fda4992 to d0eed46 Compare July 15, 2024 15:45
@elevans
Copy link
Member Author

elevans commented Jul 15, 2024

Thanks @gselzer for your review! It made it sooo much better! I addressed all your comments. Ready to merge?

Copy link
Contributor

@gselzer gselzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the new changes!

...unfortunately, I requested a couple new things 😅

tests/test_callbacks.py Outdated Show resolved Hide resolved
src/imagej/__init__.py Outdated Show resolved Hide resolved
# Add function to the list of callbacks to invoke upon start_jvm().
global _init_callbacks
_init_callbacks.append(f)
if sj.jvm_started():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like the right check, does it? Presumably, users would want to utilize an initialized ImageJ here. Is that always the case if the JVM is running? I'm assuming there's some period in between...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I lifted this from scyjava but thinking more deeply about it, the right check should be the existence of the gateway itself. So the check becomes:

global gateway
if gateway:
    f(gateway)

I'll need to test this...

Copy link
Member Author

@elevans elevans Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we agreed that we will only have a gateway attribute on the imagej module created in GUI mode, this solution no longer works/makes sense. This method works as intended when called before initializing ImageJ in GUI mode. However, if ImageJ is already initialized and the main thread consumed by the event loop then it becomes impossible to call when_imagej_starts_(), making any check of a pre-existing gateway attribute pointless. As I see it, there is no good way to detect if an ImageJ has been initialized in addition to obtaining a handle on the gateway to use with the desired callback. Thus, I decided to remove this commit and revert it back to just loading the callbacks into the _init_callbacks global list.

Before I resolve this though, what do you think @gselzer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that makes sense too.

Use a more descriptive attribute name and value for
when_imagej_starts() test.
This commit adds the pthread library loading exception to
the logger and assumes that the current thread is the main
thread if no pthread library is found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants