Codecov Report

Merging #149 (50de39d) into main (b1d7e02) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #149   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines         1717      1728   +11     
=========================================
+ Hits          1717      1728   +11     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1d7e02...50de39d. Read the comment docs.

just for the sake of completeness, can you show an concrete plugin manifest using properties and how npe2 and/or napari-imagej (or scyjava, or anything in the loop) would use the properties? i.e. what sequence of events would occur at launchtime?

just for the sake of completeness, can you show an concrete plugin manifest using properties and how npe2 and/or napari-imagej (or scyjava, or anything in the loop) would use the properties? i.e. what sequence of events would occur at launchtime?

Sure, my plan was to append napari-imagej's napari.yml to be:

name: napari-imagej
display_name: napari-imagej
properties:
  jvm_classpath:
    - 'io.scif:scifio:0.43.1'
contributions:
  commands:
    - id: napari-imagej.func
      python_name: napari_imagej.widget:ImageJWidget
      title: Run ImageJ2 commands
  widgets:
    - command: napari-imagej.func
      display_name: ImageJ2

then, in my plugin, I can call:

import logging
import imagej
from scyjava import config, jimport
from napari.plugins import plugin_manager

# -- LOGGER CONFIG -- #

_logger = logging.getLogger(__name__)
_logger.setLevel(logging.DEBUG)  # TEMP

# -- IMAGEJ CONFIG -- #

# TEMP: Avoid issues caused by https://github.com/imagej/pyimagej/issues/160
config.add_repositories(
    {"scijava.public": "https://maven.scijava.org/content/groups/public"}
)
config.add_option(f"-Dimagej.dir={os.getcwd()}")  # TEMP

for endpoint in plugin_manager.properties("jvm_classpath"):
    config.endpoints.append(endpoint)

_logger.debug("Completed JVM Configuration")

Any other plugin that uses the JVM would then initialize the JVM in a similar way, using

for endpoint in plugin_manager.properties("jvm_classpath"):
    config.endpoints.append(endpoint)

just like I did above. In this way, all plugins would initialize the JVM with the JARs needed by all files. Of course, scyjava would then be responsible for ensuring that the JAR list is valid.

Is this what you were looking for @tlambert03?

yeah, thanks. need to have a think on it. busy this week. will have a look next week and curious to hear @Carreau's thoughts too

Sure thing.

It's a significant addition so I think it warrants serious review/consideration. Happy to work with you guys to revise.

Bumping for visibility

Hi @gselzer, thanks for your patience, and thanks so much for taking a crack at this PR.

We chatted about this at the plugin dev meeting yesterday. In short, I'm not sure that a new "properties" field is the best approach here. The biggest issue here was the open-endedness of Manifest.properties. Note that it would be the first and only "dict" field in the whole manifest, which points to the fact that one could really put anything in there. And since we really only have one possible use case in mind (the case of jvm_classpath), I'm not sure it warrants such an open-ended field that could lead to abuse/confusion.

Because this case of a shared global singleton is unlikely to have too many other similar cases in the near future, we're inclined to just provide you with a jvm_classpath field directly (rather than just the "convention" of using 'jvm_classpath' inside of properties. But I'd still like to have a better picture in my head about how this is going to work with/for scyjava. The example you gave above ...

for endpoint in plugin_manager.properties("jvm_classpath"):
    config.endpoints.append(endpoint)

still seems to have the issue of "whoever gets to the JVM first wins", (though it at least provides a place for everyone to share their needs). As a side question, why can't we use jpype.addClassPath to add these things after the JVM has been initialized?

If having a jvm_classpath field directly in the manifest would solve this, we're definitely open to it, but I think I still need a clearer picture of how this would all play out with scyjava and other packages. Would you and @ctrueden perhaps have time to zoom about this? I suspect it would be much easier to come up with something that way.

And since we really only have one possible use case in mind (the case of jvm_classpath), I'm not sure it warrants such an open-ended field that could lead to abuse/confusion.

Fair point. I figured that its open-endedness was a benefit, but yes, you are right that maybe the use cases do not warrant the scope of this solution.

As a side question, why can't we use jpype.addClassPath to add these things after the JVM has been initialized?

That...is a fantastic question. Maybe I misunderstood something @ctrueden said about adding classes after JVM startup. The following works on my machine, after downloading a standalone JAR like parsington:

(scyjava) : gselzer@gselzer-OptiPlex-5090 ~/code/scijava/scyjava (master) [scyjava] »
python                                                                                                                                                                                                                                                  #  0 {2022-04-21 9:11:25}
Python 3.10.4 | packaged by conda-forge | (main, Mar 24 2022, 17:39:04) [GCC 10.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import scyjava
>>> scyjava.start_jvm()
>>> scyjava.config.add_classpath('/home/gselzer/Downloads/parsington-3.0.0.jar')
>>> scyjava.jimport('org.scijava.parsington.eval.Evaluator')
<java class 'org.scijava.parsington.eval.Evaluator'>
>>> 

Note that scyjava.config.add_classpath just calls jpype.addClassPath under the hood

I think the problem is that scijava.config.endpoints, the preferred way of adding JARs to the classpath, is only added to the classpath in scyjava.start_jvm(). scijava.config.endpoints is preferred as it also adds the dependencies of all entries in the list; you'd have to resolve dependencies yourself if you use scyjava.config.add_classpath

I am starting to wonder why we need napari's help on this myself 😆

@ctrueden why can't we just add more options (namely endpoint support/dependency resolution) to scyjava.config.add_classpath?

@ctrueden why can't we just add more options (namely endpoint support/dependency resolution) to scyjava.config.add_classpath?

Dynamic classpath extension is something that @hanslovsky and I spent some time trying to do via custom class loaders back in the pyjnius days, but we weren't able to get it working due to a bug/limitation in pyjnius. Since JPype 1.2.0 (December 2020-11-29), the addClassPath function supports dynamic class loading in this fashion, yeah. This could be a great solution to avoid the wrinkles with static in-advance classpath building. However, it needs to be thoroughly tested, particularly with ImageJ 1.x functionality:

The default loader for JPype class is org.jpype.classloader.DynamicClassLoader.

But ImageJ has its own custom class loader for its plugins, and I am not sure how these two things would interact in different scenarios (the two major ones being a local ImageJ2/Fiji installation, and a jgo-based one with Maven coordinates).

If we determine that JPype's addClassPath will fully suit our needs, we would need to rework jgo and scyjava a bit:

• Currently, scyjava.config is intended to be a bag of configuration state, such that calling its mutators should not have major side effects other than changing that state, whereas the proposed change in approach would mean leaning on the fact that e.g. scyjava.config.add_classpath actually affects an already-running JVM.
• It's not enough to use addClassPath; we need to be able to specify additional endpoints and have jgo resolve those, mixing them into the existing environment, and then add the difference in state between the old and new environments (i.e. the new JAR files) into the running JVM's classpath. There are cases where this will clearly work, cases where it clearly won't, and some gray area cases where we could make a best effort but things might not work depending on library changes.
  • For example, if someone builds a jgo environment with net.imagej:imagej and then mixes in net.imagej:imagej-common, there is already an imagej-common in the former environment, but now we are bringing in the newest release of imagej-common on top of that, but adding it to the existing classpath will not have priority over the maybe older imagej-common, so you'll then have a mix of imagej-common classes at different versions on your classpath, which is bad.
  • There is a question about whether each jvm-reliant napari plugin could use its own class loader for its particular classes, to fully avoid the problem of overlapping state. I assume (without checking it for the moment) that this would be possible, due to the language above "The default loader..." implying you can override that default if desired. So on the one hand this would be great, each plugin could just feed its own environment and have it isolated via its own class loader hierarchy. But on the other hand is bad in that two JVM-reliant napari plugins could then not share Java objects between them, because e.g. napari-imagej's net.imglib2.img.array.ArrayImg is no longer the same animal as napari-curtis's net.imglib2.img.array.ArrayImg—those two classes were loaded by different class loaders are not equal, and objects of one cannot be treated as objects of the other, even if the two classes are structurally identical.

So TL;DR using custom class loaders solves some problems and introduces others. The Fiji strategy until now has been to eschew custom class loaders in favor of creating a sane unified environment accessible from the system class loader whenever possible. Doing it differently for pyimagej et al. would be a major strategic change, although not unprecedented for us.

since I don't think we're going to go with the PluginManifest.properties approach specifically. I'm going to close this PR. Thanks so much for the concrete example @gselzer. let's keep up the conversation in #163