OME element implements OMEModel

[rleigh-codelibre]

This is part 1 of the proposed refactoring in https://docs.google.com/document/d/1bzczgG2QyujCsCvR2kSAGl5hj-NJuIikgFAC3fDkqvs/edit# and https://trello.com/c/hnpxmvhe/52-extended-annotation-support-pt-i

• OME implements OMEModel
• OMEModelImpl (Java) and detail::OMEModel (C++) are no longer used and are deprecated; OME implements OMEModel directly, and OMEModelObject indirectly
• MetadataRoot replaced by MetadataModel
• OMEXMLMetadataRoot replaced by OMEXMLMetadataModel

Testing:

• Check generated source changes
• Check builds remain green
• Check bioformats build with and without Use metadata store setModel in addition to setRoot bioformats#2712
• Check ome-files-cpp build with and without https://github.com/rleigh-codelibre/ome-files-cpp/tree/ome-model-base (this is PR Use MetadataModel ome-files-cpp#40)

The latter two points check that there is no API break when building without the branch, and that the new API additions work when building with the branch,

[sbesson]

Have started reviewing the impact of the changes on the Java side via a devspace propagating this PR from the model up to OMERO via Bio-Formats:

• although large, most of the generated changes for the Java metadata classes largely update the usage of the root by the usage of model and add the new methods to the interface and their implementations

• there a few whitespace diffs in the Java model sources, should we update the generated sources jobs to perform a git diff -w?

• there are unnecessary single blank lines inserted in the C++ model sources - can they be removed to simplify the review?

• Bio-Formats (without the extra branch) builds correctly both with Ant and Maven and all unit tests are passing

• on the OMERO side, the build is failing with

  /home/omero/workspace/OMERO-build/src/components/blitz/src/ome/formats/OMEROMetadataStoreClient.java:230: error: OMEROMetadataStoreClient is not abstract and does not override abstract method setModel(MetadataModel) in MetadataStore
  

Given the nature of the proposed changes, this compile error is fully expected as new methods are added to the public MetadataStore interface which is implemented in OMEROMetadataStoreClient. From this perspective, the changes proposed in this PR are certainly breaking from an API consumer perspective and we should certainly mark this work as 6.0.0-SNAPSHOT. This error further emphasizes the importance of including OMERO immediately in the development process. Like for Bio-Formats, we will need to derive a dedicated integration branch from a stable point in order to propagating this model extension.

As we are waiting for Bio-Formats 5.3.0 to derive a branch, I will look into integrating the Bio-Formats branch mentioned above into the Java build for Monday. Given that this branch should provide access to the underlying model object via the API, how can this API be tested? Are there concrete filesets/code samples which would highlight the new functionalities associated with exposing the OME Model object?

[rleigh-codelibre]

@sbesson I think that updating the job to do a whitespace diff would be useful (as an additional set of files--the full diff is still nice to have).

Regarding OMEROMetadataStoreClient, that's annoying. If there's a particular branch I can use, I'll be happy to open a PR for Monday. (This is one place Java 8 would have been useful--we could have added a no-op default implementation to the interface, saving this from needing any work in OMERO, and no compatibility issues.)

Regarding the C++ whitespace changes, I did have a try at doing this already, but didn't get it working. I'll see what I can do.

[sbesson]

@rleigh-codelibre for OMERO, I would think the easiest for now would be to create a branch similar to the Bio-Formats one mentioned in the description with the corresponding changes. We can include this branch into the build for now and then we can spin up new integration branches for this work.

[dgault]

I've gone through the code changes here for both the Java and C++ and the changes seem sensible and there was nothing that concerned me too much. The generated sources for both look to be correct also from going through diffs.

From a Bio-Formats point of view everything builds correctly and the unit tests run successfully. Ive also done some brief sanity tests with the Bio-Formats artefacts and everything seems fine on that front.

From a C++ perspective I would like to do some extra sanity tests over the weekend to ensure everything is also fine on that front.

I haven't yet had a chance to look at OMEROMetadataStoreClient either.

[sbesson]

I will try to have a closer look at this set of PRs on Monday. The revision of the inheritance between model and root classes was discussed in November as the basis for extending and registering custom annotations in the model.

One immediate API question arising from my reading of the diff in ome/bioformats#2712 and this PR. I understand the addition of getModel() to handle internally the casting when retrieving and manipulating model objects. I am more unclear about the functional role of setModel() especially with regard to setRoot(). Since a root object is now a model object as per these changes, what is the role of each setter in the apparent duplicated methods in https://github.com/openmicroscopy/bioformats/pull/2712/files#diff-2db6ac7b887de8cd4a99322df73943ceR374 and should setRoot(root) implicitly call setModel(root) instead?

[sbesson]

Reviewed this PR and the related ones (mostly from the Java side):

• all the related CI jobs (C++, Java) have been green with this set of PRs included. This also cover the various unit and integration tests at all levels 👍

• performed a minimal client test using MATLAB / Bio-Formats with an OME-TIFF as follows

   >> bfCheckJavaPath();
   >> r = bfGetReader('/Users/sbesson/Desktop/data_repo/curated/ome-tiff/ome-schemas/2016-06/bioformats-artificial/multi-channel-4D-series.ome.tif');
   >> root = r.getMetadataStore().getRoot();
  

  Checking the root object now has access to the OMEModel API allowing to retrieve model objects, links (getModelObjects, resolveReferences...) as expected

• the simplification of the internal details of the OMEXMLMetadata implementation to use only one private variable makes it easier to understand

A couple of API and implementation questions:

• in terms of integration, is Use metadata store setModel in addition to setRoot bioformats#2712 a requirement? The PR description suggests you can build the new model additions with and without? Should the PR be temporarily excluded and the Java suite relaunched to check all tests pass without it? Is it meant to be merged or mostly a way to test the new API
• related to the above and as already mentioned in OME element implements OMEModel #17 (comment), my biggest API question is related to the contract of the new setModel() API and its relationship to setRoot() ? What is the role of each API call: the internal implementation of OMEXMLMetadataImpl suggests they are both equivalent ways to set the common root/model object but the modifications to the converter and Use metadata store setModel in addition to setRoot bioformats#2712 seem to indicate setModel supplements setRoot. Does the order matter? Does each setter effectively override the other call or can I create a state with mismatching root/model objects? Also what is the impact of this change on any consumer of the metadata API, would client code need to be updated?
• implementation detail: why is OMEXMLMetadataRoot root renamed as OMEXMLMetadataRoot model in the various classes? A large portion of the generated diff is related to this renaming which does not seem necessary especially as a root is now a model as per this PR.

[rleigh-codelibre]

@sbesson To answer your questions:

ome/bioformats#2712 is not a requirement. It's there to verify that all the Java changes in this PR are functional, and that compiling and tests all pass at the bioformats level using this functionality. It's meant to be merged, but it's also deliberately open in parallel for testing this PR. I've tested that this PR works standalone locally, but you could exclude it (and the openmicroscopy PR) if you wanted to test it standalone as well. I just merged this PR into master and ran mvn install and then built bioformats metadata_files.

The relationship between setModel and setRoot. The "root" is owned by the model. This implies that setting the model also sets the root, which may subsequently be changed. For the OME-XML model, these are equivalent actions, but the PRs take care to call both methods in order as necessary to cater for any other implementations which have different semantics (likely zero exist though). So the order matters theoretically and in practice according to the design, but for the concrete case of OME-XML it doesn't matter at this moment. Ideally, we'll drop the root setting entirely in the following release, leaving only the model. We could drop it now if we wanted to break the API; it depends upon whether you want this to be usable with bioformats 5.3, or 6.0. Ideally we would delete the metadata classes entirely from bioformats, but that's a hard break with no way to deprecate things; we could perform the Root removal at the same time.

Renaming of OMEXMLMetadataRoot root to OMEXMLMetadataRoot model. This switches to using the "model" rather than the root. For compatibility, we have to have a concrete Root instance, though we treat it as a model. When we remove the Root interface, we will simply rename OMEXMLMetadataRoot model to OMEXMLMetadataModel model. It's doing the renaming of the variable and all its usage up front to indicate the intent. Not strictly necessary, but by doing all the work up front, we leave the deprecation and removal as simple and minimal followup steps.

[sbesson]

With and without ome/bioformats#2712, all Bio-Formats and OMERO server integration tests are passing.

As discussed earlier, I am still not fully convinced by the MetadataModel/OMEXMLMetadataModel layer. From a consumer standpoint, the OME object was already subclassed in ome/bioformats#505 and there is no well-defined application or use case apparently justifying this rearchitecture. On the other hand, the proposed extension of OME opens really exciting perspectives especially in the context of the upcoming additions of object factories and type registration methods.

I don't think the doubts expressed above should hold this PR being merged. Once we have a first working version of our model supporting custom annotations and as part of the roadmap towards the next major release of ome-model, let us take the time to collectively review and agree all the design choices made and codify their intent and rationale using the API and/or Sphinx documentation.

[rleigh-codelibre]

@sbesson I created https://trello.com/c/Co2hkfMJ/454-split-ome-model as a followup. I can understand not being 100% happy with the MetadataModel addition; the MetadataRoot was also a hack needed to do the C++ porting from Java. I would be more than happy to see all of them removed permanently. The only reason they exist is to allow access to the model from the multi-layered metadata stores, which completely breaks the encapsulation they provide, and if it were possible to construct the OMEXMLMetadataStore directly with the model it wraps, none of this would be needed--it's entirely a legacy of historical design decisions in the Java codebase.