scijava / scijava

Foundational libraries for scientific Java applications 🧱
https://scijava.org
7 stars 0 forks source link

Declare imglib2 functions as ops #56

Open ctrueden opened 3 years ago

ctrueden commented 3 years ago

Ensure imglib2-algorithm can declare all its functionality as ops in a dependency-light (or even dependency-free—e.g. XML? Spring? see #55) way.

Of course, the imglib2-algorithm-side work is not technically part of scijava-ops, but we want to be 100% confident the architecture will support this.

We need to decide how to consume imglib2-algorithm from Ops. Possible approaches:

  1. [CURRENT] Leave imglib2-algorithm alone. Wrap all algorithms in imagej-ops.
    • Pro: No changes needed to imglib2-algorithm. No new dependencies.
    • Con: Tons of boilerplate. Does not evolve with additions to imglib2-algorithm. Extra maintainer effort from people less close to the imglib2 project.
  2. ServiceLoader. Change imglib2-algorithm to:
    • Annotate static methods as @OpMethod / @OpField of an OpCollection.
    • Make classes implement the correct functional interface.
    • Declare all implementations according to ServiceLoader mechanism (e.g. for Java 9+, in module-info.java).
    • Pro: Automatically expose algorithms as ops for the matcher, without depending on the matcher!
    • Con: Dependency on ops functional interfaces (function, computer, inplace). Need to manually edit module-info.java to declare your op implementations (potential action-at-a-distance errors waiting to happen)—unless we impose an additional compile-time indexing mechanism of some kind.
  3. ServiceLoader with compile-time indexing.
    • As (2) above, but with scijava-indexer, or Spring indexer, or something, so you don’t have to maintain your module-info manually.
    • Pro: Reduce action-at-a-distance errors/skew.
    • Con: Another compile-dependency (but maybe can be hidden in pom-scijava! Depends where the annotation interface(s) being used live—research needed).
    • This approach can be “added on” to B perhaps, and can wait to explore till later.
  4. Go all-in on Spring, with all algorithms discoverable via Spring-based discovery (which mechanism? TBD).
    • Con: Adds big dependencies to imglib2-algorithm. (Or does it?)
    • Pro: Unified framework across more of our ecosystem, as SciJava 3 adopts Spring in general for its context-aware layers.

Additional notes:

gselzer commented 3 years ago

Time commitment: 1 week per experimental approach

gselzer commented 11 months ago

We're actually going to quickly try out option 5:

ctrueden commented 11 months ago

which minimally adds a ops.yaml that we manually author

What about keeping the javadoc in sync?

gselzer commented 11 months ago

which minimally adds a ops.yaml that we manually author

What about keeping the javadoc in sync?

@ctrueden I'm not sure what you mean about keeping the javadoc in sync. I would probably start out by pinning a version of imglib2-algorithm, and we could either copy the javadoc or just leave it out.

I'd personally love to javadoc imglib2-algorithm, but I'm very doubtful that doing that would fit within our timeframe. Keep in mind that we'd have to release the SciJava Ops Indexer so that imglib2-algorithm could depend on it, and then file a PR adding the dependency and the javadoc to imglib2-algorithm, and then get imglib2-algorithm released. I think that's a lot harder than keeping things internal.

Finally I'll note that I don't think including imglib2-algorithm is necessary for the Paper 1 milestone, but @hinerm did, and I'm not against trying this as far as proving the framework goes. So if you'd prefer to wait on doing this beyond paper 1, we can "do it better" by waiting for the releases like I mention above.

ctrueden commented 11 months ago

@hinerm @gselzer I oppose manually maintaining an ops.yaml for imglib2-algorithm, even temporarily. We have done the work in imglib2-algorithm already to add the @implNote op annotations. Since time is of the essence, we can just build the ops.yaml from the existing branch that @gselzer made, and wrap that. Then the javadoc will still be there, and this thing can be kept in sync simply by using git rebase origin/master on the existing branch and rebuilding the ops.yaml again.

hinerm commented 11 months ago

We have done the work in imglib2-algorithm already to add the @implNote op annotations

For reference.

So if you'd prefer to wait on doing this beyond paper 1, we can "do it better" by waiting for the releases like I mention above

@gselzer @ctrueden learning from ImageJ-Ops I think that relying on code modifications for adoption of ops is a mistake. I am personally uncomfortable not having a proof-of-concept showing dependency-free inclusion of a library in the paper. I am not advocating for a manually curated yaml, but if it came to it I think that would be better than nothing; I think a program to auto-generate a yaml from a library is a perfect use-case to drive and solidify SJOps use.

ctrueden commented 11 months ago

I think a program to auto-generate a yaml from a library is a perfect use-case to drive and solidify SJOps use.

@hinerm Could you elaborate? How are you suggesting we tackle this issue for paper 1?

hinerm commented 11 months ago

@hinerm Could you elaborate? How are you suggesting we tackle this issue for paper 1?

Just merge the imglib2 algorithm branch seems sufficient for this issue. I didn't know that branch existed yesterday when @gselzer made this comment.

I made a separate issue for an ops.yaml use case

hinerm commented 11 months ago

Just merge the imglib2 algorithm branch seems sufficient for this issue.

The branch is out of date now and needs updating to move away from Therapi.

Looking at the current state of the branch, resolving https://github.com/scijava/scijava/issues/163 first would allow us to improve the annotations in ImgLib2

gselzer commented 10 months ago

Note that this is very close to done, we're just waiting on the release of the SciJava Ops Indexer, so for now the issue is blocked.