Retain RichOp when resolving dependencies

gselzer · ctrueden · commit f4eba1985742 · 2022-04-13T11:46:52.000-05:00
This allows the dependencies to update the progress of their parent Task

This commit also fixes some expected answers within ProvenanceTest.
Previously, these tests asserted that only the outside (mapper) function
altered the output. But, in this case, the output was directly created
by the mapped function. Thus there were really two Ops that directly
altered the output. Now, the test reflects that.
diff --git a/scijava/scijava-ops-engine/src/main/java/org/scijava/ops/engine/impl/DefaultOpEnvironment.java b/scijava/scijava-ops-engine/src/main/java/org/scijava/ops/engine/impl/DefaultOpEnvironment.java
@@ -477,10 +477,9 @@ private OpInstance<?> instantiateOp(final OpCandidate candidate,
 		Hints hints)
 	{
 		final List<RichOp<?>> conditions = resolveOpDependencies(candidate, hints);
-		final List<InfoChain> depChains = conditions.stream().map(op -> op
-			.metadata().info()).collect(Collectors.toList());
-		final InfoChain chain = new InfoChain(candidate.opInfo(), depChains);
-		return chain.op();
+		InfoChain adaptorChain = new DependencyInstantiatedInfoChain(candidate
+			.opInfo(), conditions);
+		return adaptorChain.op();
 	}
 
 	/**
diff --git a/scijava/scijava-ops-engine/src/main/java/org/scijava/ops/engine/impl/DependencyInstantiatedInfoChain.java b/scijava/scijava-ops-engine/src/main/java/org/scijava/ops/engine/impl/DependencyInstantiatedInfoChain.java
@@ -25,10 +25,7 @@ public DependencyInstantiatedInfoChain(OpInfo info, List<RichOp<?>> dependencies
 
 	@Override
 	protected Object generateOp() {
-		List<Object> dependencyInstances = dependencies.stream() //
-			.map(d -> d.op()) //
-			.collect(Collectors.toList());
-		return info().createOpInstance(dependencyInstances).object();
+		return info().createOpInstance(dependencies).object();
 	}
 
 	public static List<InfoChain> chainsFrom(List<RichOp<?>> dependencies) {
diff --git a/scijava/scijava-ops-engine/src/test/java/org/scijava/ops/engine/impl/ProvenanceTest.java b/scijava/scijava-ops-engine/src/test/java/org/scijava/ops/engine/impl/ProvenanceTest.java
@@ -134,9 +134,9 @@ public void testMappingProvenance() {
 		Thing out = ops.op("test.provenanceMapper").input(array).outType(
 			Thing.class).apply();
 
-		// Assert only one execution upon this Object
+		// Assert two executions upon this Object, once from the mapped function, once from the mapper
 		List<RichOp<?>> executionsUpon = ops.history().executionsUpon(out);
-		Assert.assertEquals(1, executionsUpon.size());
+		Assert.assertEquals(2, executionsUpon.size());
 	}
 
 	@Test
@@ -176,15 +176,15 @@ public void testMappingProvenanceAndCaching() {
 		Thing out = ops.op("test.provenanceMapper").input(array).outType(
 			Thing.class).apply(hints);
 
-		// Assert only one run of the Base Op
+		// Assert that two Ops operated on the return.
 		List<RichOp<?>> history = ops.history().executionsUpon(out);
-		Assert.assertEquals(1, history.size());
+		Assert.assertEquals(2, history.size());
 
-		// Run the mapped Op, assert still one run on the mapper
+		// Run the mapped Op, assert still two runs on the mapper
 		Thing out1 = ops.op("test.provenanceMapped").input(2.).outType(Thing.class)
 			.apply(hints);
 		history = ops.history().executionsUpon(out);
-		Assert.assertEquals(1, history.size());
+		Assert.assertEquals(2, history.size());
 		// Assert one run on the mapped Op as well
 		history = ops.history().executionsUpon(out1);
 		Assert.assertEquals(1, history.size());
diff --git a/scijava/scijava-ops-engine/src/test/java/org/scijava/ops/engine/progress/DefaultProgressTest.java b/scijava/scijava-ops-engine/src/test/java/org/scijava/ops/engine/progress/DefaultProgressTest.java
@@ -7,7 +7,9 @@
 import org.junit.Assert;
 import org.junit.Test;
 import org.scijava.ops.engine.AbstractTestEnvironment;
+import org.scijava.ops.spi.Op;
 import org.scijava.ops.spi.OpCollection;
+import org.scijava.ops.spi.OpDependency;
 import org.scijava.ops.spi.OpField;
 import org.scijava.plugin.Plugin;
 
@@ -64,7 +66,7 @@ public void testSimpleReporter() throws InterruptedException {
 	@Test
 	public void testMultiStageReporter() throws InterruptedException {
 		// obtain the Op
-		BiFunction<Integer,Integer, Integer> op = //
+		BiFunction<Integer, Integer, Integer> op = //
 			ops.op("test.progressReporter") //
 				.inType(Integer.class, Integer.class) //
 				.outType(Integer.class) //
@@ -82,10 +84,47 @@ public void testMultiStageReporter() throws InterruptedException {
 
 	}
 
+	@Test
+	public void testDependentReporter() throws InterruptedException {
+		// obtain the Op
+		Function<Integer, Integer> op = //
+			ops.op("test.dependentProgressReporter") //
+				.inType(Integer.class) //
+				.outType(Integer.class) //
+				.function();
+
+		int numIterations = 100;
+		Progress.addListener(op, (t) -> {
+			testProgress(t.progress(), 3);
+		});
+		Thread t = new Thread(() -> op.apply(numIterations));
+		t.start();
+		t.join();
+		Assert.assertEquals(3, this.numUpdates);
+
+	}
+
 	private int numUpdates = 0;
 
 	private void testProgress(double progress, int numIterations) {
 		Assert.assertEquals((double) ++numUpdates / numIterations, progress, 1e-6);
 	}
 
 }
+
+@Plugin(type = Op.class, name = "test.dependentProgressReporter")
+class DependentProgressReportingOp implements Function<Integer, Integer> {
+
+	@OpDependency(name = "test.progressReporter")
+	private Function<Integer, Integer> dependency;
+
+	@Override
+	public Integer apply(Integer t) {
+		Progress.defineTotalProgress(0, 3);
+		for (int i = 0; i < 3; i++) {
+			dependency.apply(t);
+		}
+		return 3 * t;
+	}
+
+}

Original file line number	Diff line number	Diff line change
`@@ -477,10 +477,9 @@ private OpInstance<?> instantiateOp(final OpCandidate candidate,`
`477`	`477`	`Hints hints)`
`478`	`478`	`{`
`479`	`479`	`final List<RichOp<?>> conditions = resolveOpDependencies(candidate, hints);`
`480`		`- final List<InfoChain> depChains = conditions.stream().map(op -> op`
`481`		`- .metadata().info()).collect(Collectors.toList());`
`482`		`- final InfoChain chain = new InfoChain(candidate.opInfo(), depChains);`
`483`		`- return chain.op();`
	`480`	`+ InfoChain adaptorChain = new DependencyInstantiatedInfoChain(candidate`
	`481`	`+ .opInfo(), conditions);`
	`482`	`+ return adaptorChain.op();`
`484`	`483`	`}`
`485`	`484`
`486`	`485`	`/**`