Skip to content

Conversation

@gselzer
Copy link
Member

@gselzer gselzer commented Aug 2, 2021

This PR introduces a ServiceLoader-based Discoverer, currently able to discover Ops and OpCollections. Suppose we want make a class FooOp in module com.example.foo discoverable by ServiceLoaderDiscoverer; this can be accomplished by following these steps:

  1. Make Cls implement Op or OpCollection:
class Cls implements Op {}
  1. In module-info.java, provide Op with Cls:
module com.example.foo {
  provides org.scijava.ops.spi.Op with Cls;
}

This PR also adds OpClass to the SPI module, intended to house Op metadata normally stored in a @Plugin annotation, as well as an Op implementation in the SciJava Ops Engine module as a proof of concept.

TODO:

@gselzer
Copy link
Member Author

gselzer commented Aug 2, 2021

NB: An answer to the question: "Why must this depend on SciJava Ops SPI?:

The big reason is to be able to discover OpCollections. We need a marker interface for the ServiceLoader to use in order to discover OpMethods or OpFields. Since we already need scijava-ops-spi for the OpMethod/OpField annotations, we might as well implement OpCollection. But, let's assume that we didn't care about this, and focus only on exposing Classes that we wish to indicate should be Ops.

The ServiceLoader paradigm requires us to provide some interface to make an implementation exposed. Since Ops must be FunctionalInterfaces, we then have but two choices:

  1. Provide the FunctionalInterface (e.g. Function)
  2. Provide Op

One downside of (1) is that we may pick up Functions that were not intended to become Ops. Of course, since Ops need to have the OpClass annotation anyways, we could filter out any Functions that don't have annotations, so this part could be avoided. The other downside is that this is not extensible: for the ServiceProvider to discover any FunctionalInterfaces we would have to modify this module-info, and thus introduce a runtime dependency on scijava-function or whatever other dependency has the interface. This is not ideal

For these reasons, it is simplest to just use Op and OpCollection. Thoughts?

@gselzer gselzer force-pushed the scijava/scijava-ops/service-loading branch 2 times, most recently from 793f41a to df5acfb Compare August 6, 2021 15:24
@gselzer gselzer force-pushed the scijava/scijava-ops/service-loading branch from df5acfb to fc9874d Compare September 14, 2021 19:42
@ctrueden ctrueden self-assigned this Nov 12, 2021
@ctrueden ctrueden self-requested a review November 12, 2021 06:29
@gselzer gselzer force-pushed the scijava/scijava-ops/service-loading branch from fc9874d to ededd13 Compare November 16, 2021 19:13
Adds a Discoverer responsible for looking up implementations via
ServiceLoader
It is unfortunate to make this change on this branch, but after writing
SciJava Ops ServiceLoader, it became clear that SciJava Ops Discovery
has nothing to do with Ops. Thus it becomes SciJava Discovery (without
the "Ops). SciJava Ops ServiceLoader, however, needs the Ops name
because of its module-info.java
@ctrueden ctrueden force-pushed the scijava/scijava-ops/service-loading branch from ededd13 to f08cb58 Compare November 20, 2021 02:38
Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

This was a great first step toward using ServiceLoader as an option for discovery of ops. We have discussed and iterated the design several times since then, but the latter work is built on this first iteration, so I am merging this as is, with the understanding that the ServiceLoader-based discovery evolves substantially from this point forward.

@ctrueden ctrueden merged commit 451ace5 into main Nov 20, 2021
@ctrueden ctrueden deleted the scijava/scijava-ops/service-loading branch November 20, 2021 02:49
@ctrueden ctrueden removed their assignment Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

3 participants