Scijava/scijava javadoc parser/javadoc to yaml #79

gselzer · 2023-06-29T18:31:53Z

This PR introduces a new SciJava annotation processor to replace the usage of therapi. The processor creates YAML files describing all implementations (identified with an @implNote tag), for use in SciJava 3.

The YAML created by the processor is designed for usage beyond Ops, and for robust declaration of implementations from arbitrary, cross-langauge sources. Each implementation must contain the following:

An implementation type. Implementations are binned by their implementation type into type.yaml files (for example, op.yaml contains all of the op implementations discovered by the processor).
An implementation source, a URI that some yaml reader could use to get an instance of the implementation. For example, this could be a FQCN in Java, along with a scheme identifying that the URI is a Java class scheme.
A map of tags, containing type-specific information. For ops, this includes the op's name, it's parameter information, priority, etc.

As an example, here's an Op:

	/**
	 * A Field Op
	 * @implNote op name=example.mul
	 * @input a the first double
	 * @input b the first double
	 * @output the product
	 */
	public final BiFunction<Double, Double, Double> multiply = (a, b) -> a * b;

And here is the generated YAML, placed into op.yaml:

  - op:
      name: example.mul
      priority: 0.0
      source: javaField:/org.scijava.ops.engine.yaml.ops.YAMLFieldOp$multiply/v0.0
      parameters:
        - input: in1
          description: The first input
        - input: in2
          description: The second input
        - output: out
          description: The product

Although the build completes and passes tests, this PR is a draft as there are a couple things left to do:

Polish the annotation processor, and the yaml readers, and the schema we're currently using - while ImageJ Ops2 tests it, I'm sure there are still edge cases and inefficiencies.
Determine whether we need @hinerm's ruby script in pom.xml to merge the *.yaml files between source and tests - @ctrueden seemed to think that we don't need to do that. My experience is that it is necessary, though..

ctrueden · 2023-06-29T18:35:58Z

What I suggested was to make the path to the YAML for src/test distinct, like op.test.yaml, and check whether the getResources call is able to discover it then. I.e. is the problem a clash between multiple YAML files of the same path in the same project? Or is the issue merely discovery of resources in target/test-classes in general, maybe due to JPMS issues? If the former, using a different name works around the problem.

gselzer · 2023-07-10T19:44:06Z

Closes scijava/scijava#65

ctrueden

Thank you so much for working on this, @gselzer! I'm excited to have ops driven uniformly by YAML metadata. But it looks like this PR is not quite ready for review yet:

The description has empty code blocks for the op and corresponding YAML.
The commit history has WIPs.

gselzer · 2023-07-17T15:19:55Z

@ctrueden I edited my PR to add the example Op and resulting YAML, and squashed together all of the WIP commits into a single commit, because things really weren't passing the build until the very end. Should be good to go now!

hinerm · 2023-08-17T14:44:27Z

Build with:
mvn clean install -pl scijava/scijava-javadoc-parser -am; mvn clean install

Questions:

Can everything build in on multi-module build? Or does the javadoc parser need to be in its own component?
Is there an easier way to debug the annotation processor than logging output?

hinerm · 2023-08-18T16:37:48Z

@gselzer what are the benefits to replacing the therapi mechanism for harvesting Javadoc ops? I didn't see a corresponding issue for this PR.

gselzer · 2023-08-18T16:48:26Z

@gselzer what are the benefits to replacing the therapi mechanism for harvesting Javadoc ops? I didn't see a corresponding issue for this PR.

Some benefits are described in Improve the shortcomings of therapi javadoc processing scijava#65.
A further benefit is that we no longer have to scan the entire classpath. Right now, there is no way to ask therapi "give me all javadoc'd elements that contain an @implNote annotation - we have to scan the entire classpath to do that, which actually breaks down when the classpath isn't fully populated e.g. in Fiji. It's also expensive.

There may be more, which I'm forgetting...

hinerm · 2023-08-24T14:14:44Z

@gselzer I've rebased this branch against main. Currently all the imagej-ops tests are failing, despite running the separate mvn clean install -pl scijava/scijava-javadoc-parser -am; mvn clean install commands. Can you take a look?

Also are the op parameter names being retained? Can you point me to an example of where they are used?

gselzer · 2023-08-28T19:31:36Z

@hinerm looking now, it seems to be an issue with type variables. There are some identifiers in the imagej ops2 yaml that have type variables, such as:

net.imagej.ops2.copy.Copiers.<T>copyType(T, T)

unfortunately, we are not correctly reifying the parameter types. I think that the solution is just to erase the type variables to their bounds. I'll let you know when I have a fix.

hinerm · 2023-08-29T14:44:53Z

@gselzer based on discussion today:

Identify where exceptions are being eaten for invalid op javadoc (methods without a type) and see if we can get that to fail compilation

gselzer · 2023-08-29T15:47:22Z

@gselzer based on discussion today:

* [ ]  Identify where exceptions are being eaten for invalid op javadoc (methods without a type) and see if we can get that to fail compilation

So, looking back at this, I think we should avoid failing compilation - the javadoc parser is designed to be more general than Ops, and thus we may not always want to conform to the Javadoc requirements for an Op.

What we can do is ensure that the tests fail because an Op written as a method doesn't have a type hint. Right now, if we miss a type hint, the errors look like this:

[ERROR] Tests run: 4, Failures: 0, Errors: 4, Skipped: 0, Time elapsed: 0.173 s <<< FAILURE! - in org.scijava.ops.engine.yaml.YAMLOpTest
[ERROR] testYAMLFunction  Time elapsed: 0.075 s  <<< ERROR!
java.lang.IllegalArgumentException: java.lang.RuntimeException: java.lang.RuntimeException: Op org.scijava.ops.engine.yaml.ops.YAMLMethodOp.subtract(java.lang.Double,java.lang.Double) could not be loaded: Could not load class null
	at org.scijava.ops.engine@0-SNAPSHOT/org.scijava.ops.engine.yaml.YAMLOpTest.testYAMLFunction(YAMLOpTest.java:69)
Caused by: java.lang.RuntimeException: java.lang.RuntimeException: Op org.scijava.ops.engine.yaml.ops.YAMLMethodOp.subtract(java.lang.Double,java.lang.Double) could not be loaded: Could not load class null
	at org.scijava.ops.engine@0-SNAPSHOT/org.scijava.ops.engine.yaml.YAMLOpTest.testYAMLFunction(YAMLOpTest.java:69)
Caused by: java.lang.RuntimeException: Op org.scijava.ops.engine.yaml.ops.YAMLMethodOp.subtract(java.lang.Double,java.lang.Double) could not be loaded: Could not load class null
	at org.scijava.ops.engine@0-SNAPSHOT/org.scijava.ops.engine.yaml.YAMLOpTest.testYAMLFunction(YAMLOpTest.java:69)
Caused by: java.lang.NullPointerException: Cannot invoke "String.equals(Object)" because "name" is null
	at org.scijava.ops.engine@0-SNAPSHOT/org.scijava.ops.engine.yaml.YAMLOpTest.testYAMLFunction(YAMLOpTest.java:69)

[ERROR] testYAMLClass  Time elapsed: 0.001 s  <<< ERROR!
org.scijava.ops.api.features.OpMatchingException: No MatchingRoutine was able to produce a match!
	at org.scijava.ops.engine@0-SNAPSHOT/org.scijava.ops.engine.yaml.YAMLOpTest.testYAMLClass(YAMLOpTest.java:48)

[ERROR] testYAMLInnerClass  Time elapsed: 0.001 s  <<< ERROR!
org.scijava.ops.api.features.OpMatchingException: No MatchingRoutine was able to produce a match!
	at org.scijava.ops.engine@0-SNAPSHOT/org.scijava.ops.engine.yaml.YAMLOpTest.testYAMLInnerClass(YAMLOpTest.java:55)

[ERROR] testYAMLMethod  Time elapsed: 0 s  <<< ERROR!
org.scijava.ops.api.features.OpMatchingException: No MatchingRoutine was able to produce a match!
	at org.scijava.ops.engine@0-SNAPSHOT/org.scijava.ops.engine.yaml.YAMLOpTest.testYAMLMethod(YAMLOpTest.java:62)

I think this is good enough, for now.

gselzer

Reviewed a little bit!

gselzer · 2023-09-07T20:50:38Z

.github/build.sh

 #!/bin/sh
 curl -fsLO https://raw.githubusercontent.com/scijava/scijava-scripts/master/ci-build.sh
-mvn -Djavadoc.skip -pl scijava/scijava-taglets clean install
+mvn -Djavadoc.skip -pl scijava/scijava-taglets,scijava/scijava-javadoc-parser -am clean install


I'd really prefer to get these modules out of the incubator ASAP - @ctrueden the taglets, at least, seem pretty polished - should we consider migrating them?

imagej/imagej-ops2/src/main/java/net/imagej/ops2/copy/Copiers.java

pom.xml

gselzer

Phew! What a PR!

scijava/scijava-common3/src/main/java/org/scijava/common3/Classes.java

gselzer · 2023-09-08T14:00:45Z

scijava/scijava-javadoc-parser/README.md

We should change the readme to reflect the new project

And should also remember to attribute therapi for the starting code. Check their license

scijava/scijava-javadoc-parser/pom.xml

scijava/scijava-javadoc-parser/src/main/java/org/scijava/javadoc/parser/ImplClassData.java

scijava/scijava-javadoc-parser/src/main/java/org/scijava/javadoc/parser/ImplData.java

.../scijava-ops-engine/src/main/java/org/scijava/ops/engine/yaml/AbstractYAMLOpInfoCreator.java

scijava/scijava-ops-engine/src/main/java/org/scijava/ops/engine/yaml/YAMLOpInfoDiscoverer.java

...ava/scijava-ops-engine/src/main/resources/META-INF/services/org.scijava.discovery.Discoverer

scijava/scijava-ops-engine/src/test/java/org/scijava/ops/engine/yaml/ops/YAMLClassOp.java

gselzer · 2023-09-12T15:13:55Z

Some additional thoughts from discussing with @hinerm and @ctrueden:

Let's rename it the SciJava Op Indexer, and specialize it on Ops. We can always copy the code to make something broader than that.
Let's try to make the type tag optional for methods - if the user leaves it out, then we can map to it the appropriate Computer or Function.
Let's move a lot of the parameters out of the tags map, and leave it for only truly optional stuff (like the OpMethod type) - authors, name, parameters, etc. are all required for all Ops, and it's fine since we are specializing on Ops.

hinerm · 2023-09-12T18:59:39Z

@gselzer with #86 merged the docs will have to be updated as well. I think they can be as part of this PR, or split out to a new issue/branch if you prefer

Wasn't being used

It wasn't used

No real reason to keep it as a field

gselzer mentioned this pull request Jul 10, 2023

Consider whether to encapsulate javadoc access into a dedicated module scijava/scijava#89

Closed

gselzer force-pushed the scijava/scijava-javadoc-parser/javadoc-to-yaml branch from ec94780 to 33a339e Compare July 11, 2023 16:48

gselzer marked this pull request as ready for review July 11, 2023 17:01

ctrueden requested changes Jul 15, 2023

View reviewed changes

gselzer force-pushed the scijava/scijava-javadoc-parser/javadoc-to-yaml branch from 33a339e to 2c67649 Compare July 17, 2023 15:08

hinerm force-pushed the scijava/scijava-javadoc-parser/javadoc-to-yaml branch from 899a953 to 294dc26 Compare August 18, 2023 17:53

gselzer linked an issue Aug 23, 2023 that may be closed by this pull request

Improve the shortcomings of therapi javadoc processing scijava/scijava#65

Closed

hinerm force-pushed the scijava/scijava-javadoc-parser/javadoc-to-yaml branch from 294dc26 to 95fb6c9 Compare August 24, 2023 14:13

gselzer mentioned this pull request Aug 28, 2023

Minimize and proof SciJava Ops API & SPI #90

Merged

5 tasks

gselzer force-pushed the scijava/scijava-javadoc-parser/javadoc-to-yaml branch from 90f156f to d3fa2a6 Compare August 29, 2023 13:43

gselzer force-pushed the scijava/scijava-javadoc-parser/javadoc-to-yaml branch from d3fa2a6 to 0574e21 Compare August 29, 2023 15:17

hinerm mentioned this pull request Aug 30, 2023

Write wiki-like documentation pages #86

Merged

This was referenced Sep 7, 2023

Dive into the performance impacts of LazilyGeneratedMethodParameterData scijava/scijava#131

Closed

Ops unit tests are too slow scijava/scijava#102

Closed

gselzer commented Sep 7, 2023

View reviewed changes

gselzer commented Sep 8, 2023

View reviewed changes

gselzer force-pushed the scijava/scijava-javadoc-parser/javadoc-to-yaml branch from f838758 to b8f0cc7 Compare September 12, 2023 15:47

gselzer force-pushed the scijava/scijava-javadoc-parser/javadoc-to-yaml branch from 2bf8bec to 35377ce Compare September 14, 2023 17:32

gselzer added 27 commits October 2, 2023 18:58

Enable YAML Ops to infer their type

88fb80a

Require correct no. params for implNote ops

ff90eb5

Validate Ops declared in javadoc

fe48c04

Add Op version to the parser

c00bc9d

Fix indentation

14a7a84

Rename Annotation Processor

384e986

Add comment explaining the false return

5e00054

Combine OpImplNoteParser and YamlJavadocBuilder

53c5771

Remove commented out log message

712911d

Update Services

d5d546c

Add LICENSE for adapted Therapi code

8fc0515

Remove InvalidOpJavadocException

c3b2556

Wasn't being used

Various small clean-up changes

da48d44

Format module-info.java

44cca44

Clean up OpDependencyMemberParsers

11c1c0a

Remove TODO in YAML version parsing

322571b

Throw IAE when Op yaml has no name

57ba555

Partition name parser into its own method

561fafc

Remove hints from method

5fd2022

Remove SciJava Parse2 dependency

c5f6ac3

It wasn't used

OpFieldImplData: Document parseAdditionalTags

6f2acd1

OpImplData: Javadoc for parsing additional tags

009a4fa

OpImplNoteParser: Convert Field to Method

b2f181a

No real reason to keep it as a field

OpMethodImplData: simplify

51e2608

ProcessingUtils: Add documentation

6608194

Write README for the Ops Indexer

fb6e7fe

Improve ImplNote parser regex

e82f4e8

gselzer force-pushed the scijava/scijava-javadoc-parser/javadoc-to-yaml branch from 2c6286e to e82f4e8 Compare October 2, 2023 23:58

gselzer merged commit 753836d into main Oct 3, 2023

gselzer deleted the scijava/scijava-javadoc-parser/javadoc-to-yaml branch October 3, 2023 00:52

Scijava/scijava javadoc parser/javadoc to yaml #79

Scijava/scijava javadoc parser/javadoc to yaml #79

Uh oh!

Conversation

gselzer commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctrueden commented Jun 29, 2023

Uh oh!

gselzer commented Jul 10, 2023

Uh oh!

ctrueden left a comment

Choose a reason for hiding this comment

Uh oh!

gselzer commented Jul 17, 2023

Uh oh!

hinerm commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hinerm commented Aug 18, 2023

Uh oh!

gselzer commented Aug 18, 2023

Uh oh!

hinerm commented Aug 24, 2023

Uh oh!

gselzer commented Aug 28, 2023

Uh oh!

hinerm commented Aug 29, 2023

Uh oh!

gselzer commented Aug 29, 2023

Uh oh!

gselzer left a comment

Choose a reason for hiding this comment

Uh oh!

gselzer Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gselzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gselzer Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

gselzer Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gselzer commented Sep 12, 2023

Uh oh!

hinerm commented Sep 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gselzer commented Jun 29, 2023 •

edited

Loading

hinerm commented Aug 17, 2023 •

edited

Loading