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.

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

bfoptions: Add ability to read options from file #3605

Merged
merged 7 commits into from
Sep 30, 2020

Conversation

dgault
Copy link
Member

@dgault dgault commented Aug 14, 2020

This is an initial prototype of the potential functionality for persisting options using a .bfoptions file as discussed at the formats meeting this week.

If a reader detects a file in the same folder as the currentId with the same name and extension .bfoptions then the options will be parsed from the file

An example of an options file might be:

zeissczi.attachments=true
zeissczi.autostitch=true
  • This is very much an early prototype for discussion but might also be useful for IDR testing
  • At the moment this is limited to reading options from file and does not create the file for you
  • It is also limited to readers and not writers
  • Further error and exception handling would also be needed

Copy link
Member

@melissalinkert melissalinkert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 overall, just a couple of minor questions.

@dgault
Copy link
Member Author

dgault commented Aug 19, 2020

Added a new commit to throw a warning if the options in the bfoptions file are incorrect. This is one possible implementation of the requested functionality as discussed in the last formats meeting, initially using CZI as an example.

@sbesson
Copy link
Member

sbesson commented Aug 20, 2020

Some preliminary functional testing of the options file was performed in the context of a concrete reader - see #3606 (comment).

A few additional comments:

  • the initial implementation looks good and also adds some useful introspection API. As mentioned in the companion PR, I see adding the options file to the used file list as the next step to allow the testing in OMERO
  • conflicting options:
    • I tested the following command-line scenario /bio-formats-build/bioformats/tools/showinf -nopix /uod/idr/repos/curated/scanr/public/idr0009/0307-10--2007-05-30/experiment_descriptor.dat -option scanr.skip_missing_wells true with an option file containing scanr.skip_missing_wells=false. This returns 384 series so it looks like the option file is taking precedence at the moment. I would have expected command-line options to take precedence if specified explicitly i.e. default reader option < fileset option < command-line option
    • the consideration above makes me wonder how/whether this should behave in the ImageJ/Fiji environment since reader options are always overriden by the Configuration window?
  • naming scheme of the options file: the current implementation uses <master_file_name_without_extension>.bfoptions e.g. `experiment_descriptor.bfoptions:
    • should this file be visible or hidden e.g. like the Memoizer which uses .<master_file>.bfmemo?
    • are all the readers always returning to the same master file for a given fileset? Otherwise this would make the behavior dependent on the file passed to setId?

@sbesson
Copy link
Member

sbesson commented Sep 25, 2020

See #3606 (comment) for an example of testing of the import worfklow with an options file tested the getUsedFiles API.

I have been looking into adding configuration files for non-regression tests using the same ScanR dataset with an options file setting scanr.skip_missing_wells to false. Unfortunately the gen-config command led to the following expection:

[2020-09-25 16:03:02,419] INFO  
java.lang.IllegalArgumentException: Invalid series: 357
	at loci.formats.FormatReader.setCoreIndex(FormatReader.java:1384) ~[formats-api-6.6.0-SNAPSHOT.jar:6.6.0-SNAPSHOT]
	at loci.formats.ImageReader.setCoreIndex(ImageReader.java:759) ~[formats-api-6.6.0-SNAPSHOT.jar:6.6.0-SNAPSHOT]
	at loci.formats.ReaderWrapper.setCoreIndex(ReaderWrapper.java:588) ~[formats-api-6.6.0-SNAPSHOT.jar:6.6.0-SNAPSHOT]
	at loci.formats.ReaderWrapper.setCoreIndex(ReaderWrapper.java:588) ~[formats-api-6.6.0-SNAPSHOT.jar:6.6.0-SNAPSHOT]
	at loci.tests.testng.Configuration.populateINI(Configuration.java:585) ~[classes/:na]
	at loci.tests.testng.Configuration.<init>(Configuration.java:164) ~[classes/:na]
	at loci.tests.testng.FormatReaderTest.writeConfigFile(FormatReaderTest.java:2872) ~[classes/:na]
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:1.8.0_171]
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:1.8.0_171]
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:1.8.0_171]
	at java.lang.reflect.Method.invoke(Method.java:498) ~[na:1.8.0_171]
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:80) [testng-6.8.jar:na]
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:714) [testng-6.8.jar:na]
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:901) [testng-6.8.jar:na]
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1231) [testng-6.8.jar:na]
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:127) [testng-6.8.jar:na]
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:111) [testng-6.8.jar:na]
	at org.testng.TestRunner.privateRun(TestRunner.java:767) [testng-6.8.jar:na]
	at org.testng.TestRunner.run(TestRunner.java:617) [testng-6.8.jar:na]
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:334) [testng-6.8.jar:na]
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:329) [testng-6.8.jar:na]
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:291) [testng-6.8.jar:na]
	at org.testng.SuiteRunner.run(SuiteRunner.java:240) [testng-6.8.jar:na]
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52) [testng-6.8.jar:na]
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86) [testng-6.8.jar:na]
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1198) [testng-6.8.jar:na]
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1123) [testng-6.8.jar:na]
	at org.testng.TestNG.run(TestNG.java:1031) [testng-6.8.jar:na]
	at org.testng.TestNG.privateMain(TestNG.java:1338) [testng-6.8.jar:na]
	at org.testng.TestNG.main(TestNG.java:1307) [testng-6.8.jar:na]

As the default behavior of the reader for this dataset is to detect 357 series, I suspect the above indicates the two readers used for generating the configuration (flattened vs non-flattened) are not seeing the same options. I will take a look at this next week-end as well as test the workflow with the new FlexReader option.

Another aspect of this API is to decide how it affects serialization i.e. should a cached reader detect mismatching options and be able to invalidate itself?

Overall, there are a few unanswered questions but also at the same time a few open PR which depend on this new API esp. to declare their available options. I am considering getting this merged, tag a milestone release, start consuming it in various workflows and use cases and report blockers or required API/implementation changes via issues. Proposing to discuss the roadmap to Bio-Formats 6.6.0 as part of Monday's formats meeting.

@sbesson
Copy link
Member

sbesson commented Sep 29, 2020

As a summary of Monday's discussion, leaving one more day for a first round of feedback on this API addition. Barring objections, I will start integrating the first round of options PR so that follow-up PRs can be opened. Overall we are currently tending towards a mid-October release of Bio-Formats 6.6.

@sbesson sbesson closed this Sep 30, 2020
@sbesson sbesson reopened this Sep 30, 2020
@sbesson sbesson merged commit e4778a5 into ome:develop Sep 30, 2020
String p = f.getParent();
String n = f.getName();
if (n.indexOf(".") >= 0) {
n = n.substring(0, n.indexOf("."));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While working on #3617, I realized one downside of this naming pattern is that the option filename is not unique. Thinking of the potential scenarios, I think the two principal ones are:

  • filesets where constituting files have the same base name but different extensions e.g. drosophila.h5, drosophila.xml
  • separate filesets sharing the same basename e.g. test.tiff, test.png

I will try and test the first case but we might want to review whether the second case is a blocker. Currently I assume it would mean both filesets would share an option file.

@sbesson sbesson mentioned this pull request Oct 19, 2020
@dgault dgault added this to the 6.6.0 milestone Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants