-
Notifications
You must be signed in to change notification settings - Fork 27
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
Need way for plugins to communicate about shared or singleton resources - possibly on napari startup #136
Comments
to add a specific clash here: |
Another idea we could use is a global name: napari-imagej
display_name: napari-imagej
configuration:
jvm_classpath: 'example.foo:bar:1.2.3'
jvm_classpath: 'example.foo:baz:4.5.6'
contributions:
commands:
- id: napari-imagej.func
python_name: napari_imagej.widget:ImageJWidget
title: Run ImageJ2 commands
widgets:
- command: napari-imagej.func
display_name: ImageJ2 (I'm not tied to any of these names 😅) On startup Any plugin spinning up the JVM would access the |
@gselzer I don't think YAML allows duplicate keys. So it would be: configuration:
jvm_classpath:
- 'example.foo:bar:1.2.3'
- 'example.foo:baz:4.5.6' ? And then you'd need a policy for how configuration gets combined across multiple plugins. Maybe sufficient to just say the configuration dicts get merged via something like deepmerge?
But it would be boilerplate-y to require every JVM-dependent plugin to write something like: if not scyjava.jvm_started():
scyjava.config.add_classpath(npe2.configuration['jvm_classpath']) don't you think? And error-prone, unless scyjava gets smarter about duplicate entries. I don't see a more elegant way without a "controller" style plugin or shared API... Am I missing something about your approach, @gselzer? |
Yeah, maybe. I admit I don't know much about YAML.
That will be close to what we need, yes, with a little versioning enforcement in scyjava if it isn't there already.
I don't think it's terribly boilerplate-y, if we can get it to two lines like that. If you'd prefer a one-liner, I think we can get away with scyjava.before_jvm_starts(lambda: scyjava.config.add_classpath(npe2.configuration['jvm_classpath'])) You are right about it being error-prone, but shouldn't scyjava should be able to reason about duplicate entries anyway? That seems like an issue outside of napari plugins. I'd say the nicest thing about this approach is that it does not require much work on napari's end. All napari would need to add is the ability to create a global map of settings from I wouldn't say I'm against adding these "blessed" plugins per se, I'm just not sure I see how to do it yet. |
My thinking on this issue evolved a bit after the discussion above, but I never followed up here to articulate it. What I want to do is give scyjava its own manifest mechanism, which libraries using scyjava would make use of. So then if you write a napari plugin that uses scyjava, napari doesn't need to know anything about it, because when scyjava initializes the JVM (explicitly or implicitly), it will discover and include all manifests present in site-packages or whatnot. I filed scijava/scyjava#59 to track this feature, and would suggest closing this issue in favor of that one. Ideas on potential details, pitfalls, etc., are welcome! |
@gselzer raised an important point on zulip. Here is a specific challenge for plugins using the JVM, but it hints at a broader challenge for plugins that need to share some external initialization:
@ctrueden proposed something like "kernel space" plugins (such as a
napari-jvm
plugin) that act as aggregators for related functionality, and resources that must be shared:I like that general idea, as it's extensible for arbitrary other resources. One question becomes whether
napari-jvm
would be in some way a "blessed" plugin – that is, how does everyone agree on what the kernel-space aggregator is? And doesnpe2
need to add thejava_libraries
field to the npe2 schema? or doesnapari-jvm
register a key during it's special on-startup activation event that other plugins can declare support for?thanks @gselzer and @ctrueden for raising. Curious to hear @nclack's thoughts on this particular issue.
The text was updated successfully, but these errors were encountered: