Allow granular progress updates & associated changes by gselzer · Pull Request #213 · scijava/scijava

gselzer · 2024-05-03T20:05:40Z

This PR refactors the scijava-progress module API, for the goals of simplicity and better progress reporting within dependencies, and should be squeaked in before the 1.0.0 release.

Important changes:

Progress stages have been done away with, with Tasks instead having a single tracker of elements, discrete packets of progress updates. The number of elements a Task will have is dictated during Progress.defineTotal, meaning we no longer need the Progress.setStageMax method.
The progress of Op dependencies was previously limited to a boolean value, which is not helpful for "wrapper Ops", where progress updates are most important in dependency subtasks. Dependency progress updates are now passed along to every parent using a new method Task.parent(), meaning that if an Op dependency has a dependency itself that issues an update, ProgressListeners tuned to its parent and grandparent will be notified as well. To prevent an overwhelming amount of dependencies from flooding the SciJava Ops Legacy progress listener only posts updates for tasks with no parent.
- To enable progress updates from Op dependencies, you must add the "progress.TRACK" hint (i.e. org.scijava.ops.engine.BaseOpHints.Progress.TRACK), and use Progress.defineTotal(x, y), where x is a long defining the number of elements in the parent Op and y is the total number of reporting dependency executions. E.g.

public class ParentOp implements Function<X, Y> {
  @OpDependency(name="foo.bar", hints={"progress.TRACK"})
  Function<X, Y> foo;
  @OpDependency(name="baz.bar", hints={"progress.TRACK"})
  Function<X, Y> baz;

  @Override
  public Y apply(X in) {
    Progress.defineTotal(0, 3);
    out1 = foo.apply(in);
    out2 = foo.apply(in);
    out3 = baz.apply(in);
    return out3;
  }
}

Two of the most intensive Ops, coloc.saca.heatmapZScore and deconvolve.richardsonLucy, now report progress.

As we are approaching the initial release, we should consider the API - I know @ctrueden wanted to do an API review, so I'm pinging his review as motivation to review the scijava-progress public API. I'm also asking for @elevans' opinion as an Ops developer/user - if you'd like to try your hand at adding progress to fully test the API, I'd appreciate it, but if you're busy no worries.

Points of Discussion:

Can we make it easier for progress listeners to receive updates only about top-level Ops (i.e. not Op dependencies)?
It would be slick to add progress updates to LoopBuilder constructs such that pixel-wise Ops always report progress when lifted to image level, however this anecdotally incurs very poor performance due to the constant multithreaded updates to the AtomicLong tracking progress. @tpietzsch is there anything that we could do, here or upstream in ImgLib2, to enable progress reporting? Potentially one update per chunk would be awesome - would it be possible to add API to report chunk size, for example?

TODO:

Update documentation
Add progress to any more Ops?

It's simpler and faster this way

We do not want users to call this method, as the Task does not know its listeners (i.e. Progress.update() pings ProgressListeners listening to the Task while Task.update()) does not.

Should be more efficient

While nobody would explicitly do this, it could happen programmatically

gselzer

Here are my suggestions! Thanks @gselzer for doing this work!

Op discovery can take some time - let's let users know something's happening

It is no longer used

We want to propagate progress updates up the chain of subtasks, so tasks must know their parent, at least within the scijava-progress module. To avoid publicly exposing this functionality, though, we make a new method isSubTask which allows us to optionally disregard all subtasks.

elevans

@gselzer Nice PR! Ty for this work adding progress updates to SciJava Ops. This is much appreciated for those long Ops. There a few documentation changes I'd like to see before we merge this into main. I also have some open questions that I'd like to get your thoughts on:

The progress bar/feedback only really works with Fiji correct? Is there something we can do for users that are using SciJava Ops from Python or some other non-Fiji entry-point?
The create.kernelDiffraction Op that creates the PSF for deconvolution takes a little bit of time (not much but its detectable). That might be another good Op to add progress to.
How does this progress mechanism work with external libraries?

I don't have any code suggestions. It looks very clean and well documented to my eye 👍 .

elevans · 2024-05-09T17:25:09Z


 ```

+### Defining Op Progress


The Op progress mechanism here described seems to exist primarily for SciJava Ops via Fiji. Can you add a bit about scijava-progress's relationship with Fiji?

elevans · 2024-05-09T17:27:08Z


+### Defining Op Progress
+
+By adding the `scijava-progress` module, your Op can define and update its progress, providing user value for long-running Ops. To add progress to your Op, you must add the following steps to your Op:


What kind of feedback, if any, would a user expect to receive from scijava-progress if they're using SciJava Ops via scyjava for example?

They'd receive no feedback, unless they write their own ProgressListener and register it within the Progress class.

Naively speaking, I'd assume any scijava-ops-python component could harness tqdm to record progress updates, for example.

elevans · 2024-05-09T17:39:22Z

+/**
+ * A simple summer
+ *
+ * @implNote op names="stats.sum"
+ */
+class DoubleSumOp implements Function<double[], Double> {
+    public Double apply(final double[] inArray) {
+        Progress.defineTotal(inArray.length);
+        double sum = 0;
+        for (double v : inArray) {
+            sum += v;
+            Progress.update();
+        }
+        return i;
+    }
+}


I know this is simple, but can you add comment lines and import Progress like this? It helps catch the eye. For a moment I didn't see that you added the progress bits to this example.

/** * A simple summer * * @implNote op names="stats.sum" */ import org.scijava.progress.Progress; class DoubleSumOp implements Function<double[], Double> { public Double apply(final double[] inArray) { // define total progress size Progress.defineTotal(inArray.length); double sum = 0; for (double v : inArray) { sum += v; // increment the progress Progress.update(); } return i; } }

elevans · 2024-05-09T17:48:06Z

+* For each Op dependency that you want to track, pass the Hint `"progress.TRACK"` within the `@OpDependency` annotation.
+* Replace `Progress.defineTotal(long elements)` with `Progress.defineTotal(long elements, long subTasks)`, where `subTasks` is the **total** number of times you will invoke Op dependencies annotated with `"progress.TRACK"`.


I'm unclear if the Op dependency itself needs to have Progress.defineTotal() and Progress.update(). This seems to be the case with PadRichardsonLucyTV.java and PadRichardsonLucy.java both tracking the RichardsonLucyC.java (i.e. the core RL Op) with the "progress.TRACK" Op hint. RichardsonLucyC.java itself uses the number of iterations for Progress.defineTotal() and updates the progress.
Will I need to do that for all Op dependencies that I want to track progress?

Dependencies do not need to define their progress for updates in the dependent Op. If they do not define progress, they will only update the parent progress upon their completion, though.

For example, suppose you have 2 elements worth of updates in your Op, and in between them your Op calls a dependency, which under the hood has 10 elements:

If that Op reports its progress i.e. calls Progress.upate(), the dependent Op's progress will go 0.0 -> 0.33 -> 0.36 -> 0.39 -> ... -> 0.63 -> 0.66 -> 1.0

If that Op does not report its progress the dependent Op's progress will go 0.0 -> 0.33 -> 0.66 -> 1.0

elevans · 2024-05-09T17:51:10Z

+```
+


It might be nice to have a little screen shot of the progress/task manager in Fiji running decon/saca.

elevans · 2024-05-09T18:01:05Z

+    }
+}
+```
+


Can you discuss what, if any, performance hit a developer might expect with Progress? Maybe a little section on ideal use versus what a bad use of Progress looks like. I don't think we need to go crazy, but just a flavor.

gselzer · 2024-05-09T18:53:00Z

@elevans thanks for the feedback!

The progress bar/feedback only really works with Fiji correct? Is there something we can do for users that are using SciJava Ops from Python or some other non-Fiji entry-point?

There is nothing Fiji-specific about scijava-progress. e.g. Python users could just write a new ProgressListener implementation and add it as a global listener, and it would receive progress updates in the same way that the Fiji progress bar does.

...this makes me think that we should probably do away with the ProgressListener interface, and operate on Consumer<Task>s - it would be a little less code for us.

The create.kernelDiffraction Op that creates the PSF for deconvolution takes a little bit of time (not much but its detectable). That might be another good Op to add progress to.

Sure! Do you want to try it?

How does this progress mechanism work with external libraries?

I'm not really sure what you mean by this. If you're talking about progress updates of libraries that don't depend upon scijava-progress, all Ops record binary progress updates. This means that when you invoke the Op, all ProgressListeners listening to that Op will get a ping that the invocation is 0% complete. Then all computation happens, and once that finishes all ProgressListeners will get a ping that the invocation is 100% complete. In practice, you'll just see that it's running, but you won't get granular updates until you depend on scijava-progress and do the updates described in the documentation. I don't know of a way to do any better.

If you're talking about how external libraries can register their own ProgressListeners, they'll naturally have to depend upon scijava-progress and create their own ProgressListeners.

Does that help?

elevans · 2024-05-09T21:01:58Z

Awesome thanks @gselzer for the explanations. I'm happy that you've addressed all my comments. Ready to merge!

Granular updates

0d5c5f9

gselzer added bug enhancement labels May 3, 2024

gselzer added this to the 1.0.0 milestone May 3, 2024

gselzer requested review from ctrueden and elevans May 3, 2024 20:05

gselzer self-assigned this May 3, 2024

gselzer added 2 commits May 3, 2024 15:45

Remove multi-stage tasks

edf39ab

It's simpler and faster this way

Pseudocolor: restore formatting

d7749f7

gselzer force-pushed the scijava-progress/granular-updates branch from 64ba835 to d7749f7 Compare May 3, 2024 20:46

gselzer added 4 commits May 3, 2024 16:18

Task: Use CopyOnWriteArrayList for subtasks

312ea7a

Set Task.update to protected

b98ba15

We do not want users to call this method, as the Task does not know its listeners (i.e. Progress.update() pings ProgressListeners listening to the Task while Task.update()) does not.

Store ProgressListeners with CopyOnWriteArrayList

5e8dcf7

Should be more efficient

Enable tasks with 0 elements, 0 subtasks

760a172

While nobody would explicitly do this, it could happen programmatically

gselzer commented May 6, 2024

View reviewed changes

gselzer force-pushed the scijava-progress/granular-updates branch 5 times, most recently from 2b68394 to 20de674 Compare May 6, 2024 21:21

gselzer added 7 commits May 6, 2024 16:26

Create a Task for discovering in OpEnvironment

4d594ab

Op discovery can take some time - let's let users know something's happening

Test "empty" Task behavior

8265f2e

Remove subTasksCompleted

b3fcb66

It is no longer used

Refactor Task.define -> Task.defineTotal

09d99f8

Task cleanup

e488315

Update documentation

ce00177

gselzer force-pushed the scijava-progress/granular-updates branch from 20de674 to ce00177 Compare May 6, 2024 21:26

Format

dafc9af

gselzer marked this pull request as ready for review May 6, 2024 21:28

elevans requested changes May 9, 2024

View reviewed changes

gselzer and others added 3 commits May 9, 2024 15:12

Add suggested updates to WritingYourOwnOpPackage

18eca6e

Scale and center progress example figure

ce68f72

ProgressListener -> Consumer<Task>

4246cdb

elevans merged commit 50834c4 into main May 9, 2024

gselzer deleted the scijava-progress/granular-updates branch May 9, 2024 22:00


		### Defining Op Progress

		By adding the `scijava-progress` module, your Op can define and update its progress, providing user value for long-running Ops. To add progress to your Op, you must add the following steps to your Op:

		* For each Op dependency that you want to track, pass the Hint `"progress.TRACK"` within the `@OpDependency` annotation.
		* Replace `Progress.defineTotal(long elements)` with `Progress.defineTotal(long elements, long subTasks)`, where `subTasks` is the total number of times you will invoke Op dependencies annotated with `"progress.TRACK"`.

Conversation

gselzer commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gselzer left a comment

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!

Uh oh!

Uh oh!

elevans left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gselzer commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elevans commented May 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gselzer commented May 3, 2024 •

edited

Loading

elevans left a comment •

edited

Loading

gselzer commented May 9, 2024 •

edited

Loading