bioformats icon indicating copy to clipboard operation
bioformats copied to clipboard
verified publisher icon ome

Removal of compatibility metadata classes for 6.0

Open rleigh-codelibre opened this issue 7 years ago • 4 comments 

% ls components/formats-api/src/loci/formats/meta
BaseMetadata.java    IMetadata.java      MetadataConverter.java  ModuloAnnotation.java
DummyMetadata.java   IMinMaxStore.java   MetadataRetrieve.java   OriginalMetadataAnnotation.java
FilterMetadata.java  IPyramidStore.java  MetadataStore.java      package.html

These were mostly deprecated with the move to the ome-xml library in ome-model. All of the MetadataStore/MetadataRetrieve interfaces and classes implementing them can and should be removed for a breaking 6.0 release, to allow for direct use of the real interfaces and implementations in ome-xml. Likewise for MetadataConverter.

This would need a corresponding change in OMERO to switch the loci.formats.meta package to ome.xml.meta, with no other code changes required.

Is this currently planned? Would a PR to make this change be acceptable?

rleigh-codelibre avatar Dec 07 '18 14:12 rleigh-codelibre

Hi Roger,

Answering one of your last questions, the formal removal of these classes is currently not planned in any of the upcoming OME milestones.

Overall, we agree on the mid-term vision of dropping these classes in favor of the underlying ome.xml ones when possible. Interestingly, I thought we had already deprecated these classes as part of your previous cleanup in https://github.com/openmicroscopy/bioformats/pull/2677 but it looks like we did the set of classes was never annotated as such.

Concretely there are a few considerations from our side:

  • from the Bio-Formats side, the IDR branch still contains custom changes to the MetadataConverter class giving more options when converting images with very large number of ROIs very similarly to what is done in https://github.com/ome/ome-model/pull/88 (see https://github.com/openmicroscopy/bioformats/pull/2013). We are slowly trying to reconcile these two branches and port all IDR changes back in the mainline repositories. Until this functionality is ported, removing these classes and switching to ome.xml implementations would effectively mean additional maintenance burden for the IDR work
  • we are currently starting to work on OMERO 5.5 dealing with various system requirement changes as well as reintegrating the work on the new build system. The OMERO development branch is and will keep being tested against the latest stable release of Bio-Formats (5.9.2) as well as the development branch of Bio-Formats 6 since we have not made the decision on which Bio-Formats version will be shipped yet. We are planning to “freeze” the openmicroscopy repository to complete this migration soon.
  • some classes listed above are not covered in the ome.xml package like loci.formats.meta.IMinMaxStore. Is the proposal to migrate these extensions to ome-model or keep them in this repository?

Having briefly discussed with the rest of the @openmicroscopy/formats team, formally deprecating these classes in Bio-Formats 6 and scheduling their removal in the future major release makes complete sense and feel free to open a Pull Request to that effect. The class replacement will undoubtedly lead to very large code review and likely build instabilities which will necessitate some attention for at least several days. Given the other major efforts described above on at the project level, this is not something the OME team can commit at the moment. However, it would be good to discuss how to break down this large work into more seizable pieces and plan the replacement.

sbesson avatar Dec 10 '18 20:12 sbesson

Note that I did deprecate the metadata interfaces and classes with the original migration of ome-xml to the separate ome-model repository, but the deprecation annotations were later removed for cosmetic reasons (I'm not sure this was ideal). This was not the case for MetadataConverter, because it was done later, and for the reasons detailed below. The intention was always to complete this migration when possible. Right now it's not possible to use the real interfaces and implementations in Bio-Formats clients (or Bio-Formats itself) and this is a real problem for making effective use of the model interfaces. Something which I would very much like to be able to do; it's completely impossible to do a number of interesting things with alternative or extended metadata or model implementations the way things are right now.

MetadataConverter use in IDR is not relevant for the purposes of migration. The implementation is already completely defined in ome-xml, so any IDR changes would require making in the model code and not Bio-Formats; the Bio-Formats class is nothing more than a trivial delegating wrapper. There is no reason for any code to use the Bio-Formats implementation other than the type incompatibility arising from the nasty extension of the ome-xml metadata types. That's the only reason it's still present here and not deprecated (as for the metadata interfaces). As soon as the metadata types are fully dropped, it becomes entirely pointless. Ideally, it would be dropped immediately without deprecation, since it was only done this way without deprecation due to the nastiness of the rest of the metadata class wrapping.

Classes like IMinMaxStore are irrelevant for migration. They aren't in the ome-xml model and have no equivalents to migrate to, so should remain in Bio-Formats for now I think. If they do need adding, it's a non-breaking change to add them to ome-xml in the future, so doesn't need coupling to this specific removal.

Note that the work here isn't particularly large at all. Switching Bio-Formats over to using the ome-xml types is a single pull request to switch over package namespace in use. The same applies to OMERO use of the same. Because the code is already effectively using these extended types, there is no reason to expect breakage as a result. The actual ome-xml model and metadata classes in use do not change; only the extended interfaces in the method signatures change.

All of this stuff was effectively replaced and deprecated since Bio-Formats 5.3 in 2016. (Edit: it actually dates back to 2014, well before the repo splitting.) Not fixing this for the next major release of Bio-Formats would be a significant missed opportunity.

Regards, Roger

rleigh-codelibre avatar Dec 10 '18 21:12 rleigh-codelibre

Hi @rleigh-codelibre ,

the deprecation annotations were later removed for cosmetic reasons

I couldn't find these commits/PRs, could you point me to them?

Also, do you already have branches that show the changes that you are proposing?

joshmoore avatar Dec 11 '18 10:12 joshmoore

I have prepared two branches:

https://github.com/openmicroscopy/bioformats/compare/develop...rleigh-codelibre:remove-metadata-interfaces?expand=1 https://github.com/openmicroscopy/openmicroscopy/compare/develop...rleigh-codelibre:remove-metadata-interfaces?expand=1

Both are a simple search and replace of loci.formats.(meta|ome) with ome.xml.meta for the relevant class imports, followed by a reordering of the imports for consistency, and then removal of the now obsolete classes. Other than altering the imports, there are no actual code changes here.

MetadataConverter - deprecated from delegation to removal (#2677) 474c374b38f76ba2ea141d69e6b7634b1737165f - delegate+deprecated ba5b486ca9774a585c3ca03cbf4960d66123e589 - remove in linked branch above

All other classes: 4b3da513b0ba4f3aeefef3e65feb114af3ad79e9 - migration (see note regarding no deprecation possibility) in #944 taken from #911 integration branch 1b464d3d9e9ff56b99a4654965ee5eff121e93d1 - complete migration in linked branch above 604f2198b43e558c9062b12c814c1366fa5676b0 - drop old interfaces/classes in linked branch above

The removal I mentioned isn't in the develop history. It was cherry-picked from elsewhere, and I think the deprecation stuff was removed even before that mega-PR integration branch got merged four years back, so it's not explicitly mentioned in the official git history. If I recall correctly, it's related to the note above, and to quell lots of deprecation warnings. The logistics of this migration precluded a deprecation prior to final removal.

I hope the above two branches are useful. These should allow immediate completion of the migration; additional work might be required in any downstream repositories consuming bioformats, following the same simple replacement of the metadata package in use. I'll be happy to make further changes as required.

rleigh-codelibre avatar Dec 11 '18 16:12 rleigh-codelibre