Add getters and setters for each option #190
Conversation
As demonstrated by the new 'testOptionsAPI' test, this should allow NGFF-Converter and other applications to configure the converter without constructing a list of command line arguments.
There was a problem hiding this comment.
Generally, the spirit of this PR goes alongside recent improvements in #168 as the usage of this library goes beyond the initial scope of a standalone command-line converter and evolves towards an API that can be used by downstream component like NGFF-Converter.
I had separately briefly investigated the picocli built-in API to introspect and manipulate options but it's either limited or requires to use the internal model which sounds very brittle. In that sense, exposing a proper API allowing to set and retrieve conversion parameters and chain steps together sounds like the correct approach.
As noted in the description, the primary downside of this PR is the increase in terms of lines of code especially as every new property requires a private field, a getter and a setter. There are also different schools of thoughts on the different strateg an annotating public fields would definitely be the change with the least amount of code.
One advantage of the getter/setter approach quickly mentioned in the description is the ability to add validation logic. Should logging warning be emitted when some input does not match the setter condition and is ignored?
In terms of functional testing, I tried to combine this together with glencoesoftware/raw2ometiff#97 and consume then within NGFF-Converter which is the primary driver for this features, more specifically glencoesoftware/NGFF-Converter#39. After building and publishing both libraries, NGFF-Converter was built with the following diff
diff --git a/build.gradle b/build.gradle
index 5b3b5a0..15b6fff 100644
--- a/build.gradle
+++ b/build.gradle
@@ -6,8 +6,8 @@ plugins {
version = '1.1.4'
String bfversion = "6.12.0"
-String b2rversion = "0.6.1"
-String r2oversion = "0.4.1"
+String b2rversion = "0.7.0-SNAPSHOT"
+String r2oversion = "0.5.0-SNAPSHOT"
mainClassName = 'com.glencoesoftware.convert.Launcher'
applicationName = 'NGFF-Converter'
diff --git a/src/main/java/com/glencoesoftware/convert/ConverterTask.java b/src/main/java/com/glencoesoftware/convert/ConverterTask.java
index 9608df7..2de9bdd 100644
--- a/src/main/java/com/glencoesoftware/convert/ConverterTask.java
+++ b/src/main/java/com/glencoesoftware/convert/ConverterTask.java
@@ -9,6 +9,7 @@ package com.glencoesoftware.convert;
import ch.qos.logback.classic.Level;
import com.glencoesoftware.bioformats2raw.Converter;
+import com.glencoesoftware.bioformats2raw.ZarrCompression;
import com.glencoesoftware.pyramid.PyramidFromDirectoryWriter;
import javafx.application.Platform;
import javafx.concurrent.Task;
@@ -49,9 +50,7 @@ class ConverterTask extends Task<Integer> {
RunnerExecutionExceptionHandler runHandler = new RunnerExecutionExceptionHandler();
int count = 0;
for (IOPackage job : inputFileList.getItems()) {
- CommandLine runner = new CommandLine(new Converter());
- runner.setParameterExceptionHandler(paramHandler);
- runner.setExecutionExceptionHandler(runHandler);
+ Converter ngffConverter = new Converter();
File in = job.fileIn;
File out;
if (this.interrupted || job.status == COMPLETED || job.status == ERROR) {
@@ -75,20 +74,26 @@ class ConverterTask extends Task<Integer> {
temporaryStorage = job.fileOut.getParent();
}
out = new File(Paths.get(temporaryStorage, UUID.randomUUID() + ".zarr").toString());
- Set<String> validArgs = runner.getCommandSpec().optionsMap().keySet();
- List<String> argsToUse = args.stream().filter(
- (arg) -> validArgs.contains(arg.split("=")[0])).toList();
- params = new ArrayList<>(argsToUse);
+ // Set<String> validArgs = runner.getCommandSpec().optionsMap().keySet();
+ // List<String> argsToUse = args.stream().filter(
+ // (arg) -> validArgs.contains(arg.split("=")[0])).toList();
+ // params = new ArrayList<>(argsToUse);
} else {
out = job.fileOut;
- params = new ArrayList<>(args);
+ // params = new ArrayList<>(args);
}
-
// Construct args list
+ params = new ArrayList<>(args);
params.add(0, out.getAbsolutePath());
params.add(0, in.getAbsolutePath());
String[] fullParams = params.toArray(new String[args.size()]);
-
+
+ ngffConverter.setInputPath(Paths.get(in.getAbsolutePath()));
+ ngffConverter.setOutputPath(out.getAbsolutePath());
+ ngffConverter.setCompression(ZarrCompression.raw);
+ CommandLine ngffRunner = new CommandLine(ngffConverter);
+ ngffRunner.setParameterExceptionHandler(paramHandler);
+ ngffRunner.setExecutionExceptionHandler(runHandler);
int result;
if (in.getAbsolutePath().endsWith(".zarr")) {
@@ -96,24 +101,28 @@ class ConverterTask extends Task<Integer> {
result = 0;
out = job.fileIn;
} else {
- LOGGER.info("Executing bioformats2raw with args " + Arrays.toString(fullParams));
- result = runner.execute(fullParams);
+ // LOGGER.info("Executing bioformats2raw with args " + Arrays.toString(fullParams));
+ result = ngffRunner.execute(fullParams);
}
if (result == 0 && job.outputMode == PrimaryController.OutputMode.TIFF) {
LOGGER.info("NGFF intermediate generated, converting to TIFF");
- CommandLine converter = new CommandLine(new PyramidFromDirectoryWriter());
- converter.setParameterExceptionHandler(paramHandler);
- converter.setExecutionExceptionHandler(runHandler);
- Set<String> validConverterArgs = converter.getCommandSpec().optionsMap().keySet();
- List<String> convertArgs = args.stream().filter(
- (arg) -> validConverterArgs.contains(arg.split("=")[0])).toList();
- ArrayList<String> convertParams = new ArrayList<>(convertArgs);
+ PyramidFromDirectoryWriter ometiffConverter = new PyramidFromDirectoryWriter();
+ ometiffConverter.setInputPath(Paths.get(out.getAbsolutePath()));
+ ometiffConverter.setOutputPath(Paths.get(job.fileOut.getAbsolutePath()));
+ ometiffConverter.setCompression("jpeg");
+ // Set<String> validConverterArgs = converter.getCommandSpec().optionsMap().keySet();
+ // List<String> convertArgs = args.stream().filter(
+ // (arg) -> validConverterArgs.contains(arg.split("=")[0])).toList();
+ ArrayList<String> convertParams = new ArrayList<>();
convertParams.add(0, job.fileOut.getAbsolutePath());
convertParams.add(0, out.getAbsolutePath());
String[] phase2Params = convertParams.toArray(new String[0]);
- LOGGER.info("Executing raw2ometiff with args " + Arrays.toString(phase2Params));
- result = converter.execute(phase2Params);
+ // LOGGER.info("Executing raw2ometiff with args " + Arrays.toString(phase2Params));
+ CommandLine ometiffRunner = new CommandLine(ometiffConverter);
+ ometiffRunner.setParameterExceptionHandler(paramHandler);
+ ometiffRunner.setExecutionExceptionHandler(runHandler);
+ result = ometiffRunner.execute(phase2Params);
LOGGER.info("Cleaning up intermediate files");
if (out != job.fileIn) {
FileUtils.deleteDirectory(out);The tests demonstrate the usage of the Converter callable directly using the setter/getter. Howeer these setters do not seem to be recognized when wrapping the class within a CommandLine API:
- I received a
picocli.CommandLine$MissingParameterException: Missing required parameters: '<inputPath>', '<outputPath>'and needed to pass the input/output as a string array like previously - the compression argument were also ignored
Does that meanCommandLineshould exclusively be used with a command-line utility usage in mind and the converter should use the callable directly?
Additionally exposing the setters as APIs reveals some inconsistencies across the converters. Listing a few of these inconsistencies below and we can decide for each of them to address them immediately, discard them and/or capture them as issues:
setInputPath/setOutputPathusePathas input except forConverter.setOutputPathwhich is aStringsetCompressiontakes an enum as input forConverterbut a string forPyramidFromDirectoryWriterPyramidFromDirectoryWriterusessetCompressionQualitywhich could be likely be extended to support additional options likeConverter.setCompressionProperties
Matches getOutputPath/setOutputPath, and allows for supporting other input locations in the future if we choose.
Discussed earlier today, but to summarize,
577e920 uses The |
There was a problem hiding this comment.
Retested with the latest changes in this PR as well as glencoesoftware/raw2ometiff#97 and
diff --git a/build.gradle b/build.gradle
index 5b3b5a0..15b6fff 100644
--- a/build.gradle
+++ b/build.gradle
@@ -6,8 +6,8 @@ plugins {
version = '1.1.4'
String bfversion = "6.12.0"
-String b2rversion = "0.6.1"
-String r2oversion = "0.4.1"
+String b2rversion = "0.7.0-SNAPSHOT"
+String r2oversion = "0.5.0-SNAPSHOT"
mainClassName = 'com.glencoesoftware.convert.Launcher'
applicationName = 'NGFF-Converter'
diff --git a/src/main/java/com/glencoesoftware/convert/ConverterTask.java b/src/main/java/com/glencoesoftware/convert/ConverterTask.java
index 9608df7..7600ff0 100644
--- a/src/main/java/com/glencoesoftware/convert/ConverterTask.java
+++ b/src/main/java/com/glencoesoftware/convert/ConverterTask.java
@@ -9,6 +9,7 @@ package com.glencoesoftware.convert;
import ch.qos.logback.classic.Level;
import com.glencoesoftware.bioformats2raw.Converter;
+import com.glencoesoftware.bioformats2raw.ZarrCompression;
import com.glencoesoftware.pyramid.PyramidFromDirectoryWriter;
import javafx.application.Platform;
import javafx.concurrent.Task;
@@ -44,14 +45,10 @@ class ConverterTask extends Task<Integer> {
}
@Override
- protected Integer call() throws IOException {
- RunnerParameterExceptionHandler paramHandler = new RunnerParameterExceptionHandler();
- RunnerExecutionExceptionHandler runHandler = new RunnerExecutionExceptionHandler();
+ protected Integer call() throws IOException, Exception {
int count = 0;
for (IOPackage job : inputFileList.getItems()) {
- CommandLine runner = new CommandLine(new Converter());
- runner.setParameterExceptionHandler(paramHandler);
- runner.setExecutionExceptionHandler(runHandler);
+ Converter ngffConverter = new Converter();
File in = job.fileIn;
File out;
if (this.interrupted || job.status == COMPLETED || job.status == ERROR) {
@@ -75,20 +72,13 @@ class ConverterTask extends Task<Integer> {
temporaryStorage = job.fileOut.getParent();
}
out = new File(Paths.get(temporaryStorage, UUID.randomUUID() + ".zarr").toString());
- Set<String> validArgs = runner.getCommandSpec().optionsMap().keySet();
- List<String> argsToUse = args.stream().filter(
- (arg) -> validArgs.contains(arg.split("=")[0])).toList();
- params = new ArrayList<>(argsToUse);
} else {
out = job.fileOut;
- params = new ArrayList<>(args);
}
- // Construct args list
- params.add(0, out.getAbsolutePath());
- params.add(0, in.getAbsolutePath());
- String[] fullParams = params.toArray(new String[args.size()]);
-
+ ngffConverter.setInputPath(in.getAbsolutePath());
+ ngffConverter.setOutputPath(out.getAbsolutePath());
+ ngffConverter.setCompression(ZarrCompression.raw);
int result;
if (in.getAbsolutePath().endsWith(".zarr")) {
@@ -96,24 +86,18 @@ class ConverterTask extends Task<Integer> {
result = 0;
out = job.fileIn;
} else {
- LOGGER.info("Executing bioformats2raw with args " + Arrays.toString(fullParams));
- result = runner.execute(fullParams);
+ ngffConverter.call();
+ result = 0;
}
if (result == 0 && job.outputMode == PrimaryController.OutputMode.TIFF) {
LOGGER.info("NGFF intermediate generated, converting to TIFF");
- CommandLine converter = new CommandLine(new PyramidFromDirectoryWriter());
- converter.setParameterExceptionHandler(paramHandler);
- converter.setExecutionExceptionHandler(runHandler);
- Set<String> validConverterArgs = converter.getCommandSpec().optionsMap().keySet();
- List<String> convertArgs = args.stream().filter(
- (arg) -> validConverterArgs.contains(arg.split("=")[0])).toList();
- ArrayList<String> convertParams = new ArrayList<>(convertArgs);
- convertParams.add(0, job.fileOut.getAbsolutePath());
- convertParams.add(0, out.getAbsolutePath());
- String[] phase2Params = convertParams.toArray(new String[0]);
- LOGGER.info("Executing raw2ometiff with args " + Arrays.toString(phase2Params));
- result = converter.execute(phase2Params);
+ PyramidFromDirectoryWriter ometiffConverter = new PyramidFromDirectoryWriter();
+ ometiffConverter.setInputPath(out.getAbsolutePath());
+ ometiffConverter.setOutputPath(job.fileOut.getAbsolutePath());
+ ometiffConverter.setCompression("JPEG");
+ ometiffConverter.call();
+ result = 0;
LOGGER.info("Cleaning up intermediate files");
if (out != job.fileIn) {
FileUtils.deleteDirectory(out);With this sequence of calls, I was able to convert a fake file to OME-NGFF and/or OME-TIFF with various compression schemes (uncompresed for Zarr, zlib or JPEG for TIFF).
From the diff above, the outstanding requirement is that in the absence of the CommandLine wrapper, we need some mechanism to communicate the conversion status. Immediate suggestion would be to update the converter class to implement Callable<Integer> and have call return the exit code.
|
I'd expect |
|
I am having second thoughts on the value of the Reading the documentation, a |
|
I don't feel particularly strongly that |
| @@ -128,13 +128,107 @@ public class Converter implements Callable<Void> { | |||
| /** NGFF specification version.*/ | |||
| public static final String NGFF_VERSION = "0.4"; | |||
|
|
|||
| private volatile Path inputPath; | |||
| private volatile String outputLocation; | |||
There was a problem hiding this comment.
Any reason to keep outputLocation or is it also worth unifying the internal variable name with the getter/setter?
There was a problem hiding this comment.
outputPath is defined later on, and is resolved from outputLocation as either a path on disk or an s3 location. I can rename outputLocation to outputPath if you feel that's clearer, but then would need to rename the current outputPath to something else.
| public void setTileWidth(int width) { | ||
| if (width > 0) { | ||
| tileWidth = width; | ||
| } |
There was a problem hiding this comment.
Could we add a warning in case the value is ignored (similar to resolution above)?
| public void setTileHeight(int height) { | ||
| if (height > 0) { | ||
| tileHeight = height; | ||
| } |
| public void setChunkDepth(int depth) { | ||
| if (depth > 0) { | ||
| chunkDepth = depth; | ||
| } |
| public void setMaxWorkers(int workers) { | ||
| if (workers > 0) { | ||
| maxWorkers = workers; | ||
| } |
| public void setFillValue(Short tileFill) { | ||
| if (tileFill == null || (tileFill >= 0 && tileFill <= 255)) { | ||
| fillValue = tileFill; | ||
| } |
| public void setMinImageSize(int min) { | ||
| if (min > 0) { | ||
| minSize = min; | ||
| } |
|
2234a9e should address all of the logging comments. |
There was a problem hiding this comment.
(java11) sbesson@Sebastiens-MacBook-Pro-2 bioformats2raw % ./bioformats2raw-0.7.0-SNAPSHOT/bin/bioformats2raw test.fake test.zarr -w -1 -h -1 --max_workers -1 --fill-value 512 --target-min-size -1
2023-03-30 09:30:23,256 [main] WARN c.g.bioformats2raw.Converter - Ignoring invalid tile width: -1
2023-03-30 09:30:23,259 [main] WARN c.g.bioformats2raw.Converter - Ignoring invalid tile height: -1
2023-03-30 09:30:23,259 [main] WARN c.g.bioformats2raw.Converter - Ignoring invalid worker count: -1
2023-03-30 09:30:23,260 [main] WARN c.g.bioformats2raw.Converter - Ignoring invalid fill value (must be 0-255): 512
2023-03-30 09:30:23,260 [main] WARN c.g.bioformats2raw.Converter - Ignoring invalid minimum image size: -1
(java11) sbesson@Sebastiens-MacBook-Pro-2 bioformats2raw % ls test.zarr/0/
0 1
(java11) sbesson@Sebastiens-MacBook-Pro-2 bioformats2raw % grep chunks test.zarr/0/0/.zarray
"chunks" : [ 1, 1, 1, 512, 512 ],
|
Looks reasonable to me, is there any way to fetch the CLI parameter descriptions out when using API rather than CLI mode? Having access to those would save having to duplicate the text when putting together a GUI. |
This PR doesn't attempt to do anything on that front, but it's a good idea for a next step. Moving the descriptions to a resource bundle as described here: https://picocli.info/#_description_text_in_resourcebundle might be one way to do this. |
As demonstrated by the new
testOptionsAPItest, this should allow NGFF-Converter and other applications to configure the converter without constructing a list of command line arguments.Prompted by glencoesoftware/NGFF-Converter#39 and a discussion with @erindiel, @muhanadz, @sbesson, and @emilroz.
The diff looks like a lot, but really all this is doing is:
@Optionand@Parameterannotations from the variables to the settersI had hoped that there was a nicer way to connect picocli options to corresponding API methods, but as far as I can tell this is the least bad approach.
Converteris getting to be quite large with these changes, so for a future PR it may be worth refactoring.It's probably worth double-checking
bioformats2raw --helpand a few conversions with options you would normally use (or recommend to others). If there are additional unit tests that would be helpful, let me know and I can add them.I'd expect this PR to conflict with #172, but this PR is higher priority.