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.

Sign up for GitHub

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

Jump to bottom

Declare imglib2 functions as ops #56

Open
ctrueden opened this issue Apr 29, 2021 · 10 comments · May be fixed by imglib/imglib2-algorithm#94
Open

Declare imglib2 functions as ops #56

ctrueden opened this issue Apr 29, 2021 · 10 comments · May be fixed by imglib/imglib2-algorithm#94
Assignees
@gselzer
Labels
blocked
Milestone
unscheduled

Comments

@ctrueden
Copy link
Member

ctrueden commented Apr 29, 2021 •
edited
Loading

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:

  • Make scijava-function component with ONLY the functional interfaces.
    • org.scijava.ops.function -> org.scijava.function
    • NO dependency on ServiceLoader. Just paves the way.
    • OpCollection and @OpMethod and @OpField and @OpDependency?
      • @OpMethod declares functional type—but this could potentially be inferred if we instead have people annotate @Container and @Mutable on their method parameters.
        • Another potential advantage of this could be that your component does not need to depend on org.scijava.function at all, then—only on the @Container + @Mutable annotation classes… but see next point about op name declaration...
        • Those annotations could live in scijava-ops-api in that case?
    • @OpMethod and @OpField declare fully qualified op name including namespace—maybe no elegant way to get around this.
  • As an experiment, see if imglib2-algorithm can depend on scijava-function, and the algorithms there can implement the right functional interface, and module-info can declare those algorithms as implementations.
  • Then see if you can depend on imglib2-algorithm in a downstream project (e.g. imagej-ops2) and discover all the imglib2-algorithm implementations as Ops!
  • Once we have working options, present them to the ImgLib2 maintainers to discuss tradeoffs.
@ctrueden ctrueden added this to the 1.0.0 milestone Apr 29, 2021
@gselzer
Copy link
Member

gselzer commented May 13, 2021 •
edited
Loading

Time commitment: 1 week per experimental approach

@gselzer gselzer mentioned this issue Oct 18, 2021
Merged
@hinerm hinerm mentioned this issue Apr 21, 2023
Closed
2 tasks
@gselzer
Copy link
Member

gselzer commented Nov 16, 2023

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

  • Create a new module imglib2-algorithm-ops, which minimally adds a ops.yaml that we manually author, declaring all imglib2-algorithm static methods as Ops, along with relevant boilerplate to e.g. depend on the library.

@ctrueden
Copy link
Member Author

which minimally adds a ops.yaml that we manually author

What about keeping the javadoc in sync?

@gselzer
Copy link
Member

gselzer commented Nov 16, 2023

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
Copy link
Member Author

@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
Copy link
Member

hinerm commented Nov 17, 2023

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
Copy link
Member Author

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 hinerm mentioned this issue Nov 17, 2023
Closed
@hinerm
Copy link
Member

hinerm commented Nov 17, 2023

@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
Copy link
Member

hinerm commented Dec 12, 2023

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 #163 first would allow us to improve the annotations in ImgLib2

@gselzer gselzer linked a pull request Dec 28, 2023 that will close this issue
Add Op discovery annotations to ImgLib2-algorithm imglib/imglib2-algorithm#94
Open
1 task
@gselzer gselzer added the blocked label Jan 5, 2024
@gselzer
Copy link
Member

gselzer commented Jan 5, 2024

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.

ctrueden added a commit that referenced this issue May 2, 2024
@ctrueden
Merge pull request #56 from scijava/scijava/scijava-ops-engine/clean-…
3c5837c 
…provenance-test

Clean `ProvenanceTest`
@gselzer gselzer moved this from Todo to In Progress in SciJava Ops Paper #2: Polyglot May 10, 2024
@gselzer gselzer modified the milestones: 1.0.0, unscheduled May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gselzer gselzer

Labels
blocked
Projects
SciJava Ops Paper #2: Polyglot
Status: In Progress
Milestone
unscheduled
Development

Successfully merging a pull request may close this issue.

Add Op discovery annotations to ImgLib2-algorithm imglib/imglib2-algorithm
3 participants
@ctrueden @hinerm @gselzer