feature/metadata-module

[MatteBailey]

Description

This PR pulls in our CZI to OME XSLT as a Git submodule. To ensure that the submodule is including when building, this PR also adds a custom pre-build check to setup.py and uses tox to test the produced artifact, rather than just the code in the repo.

Pull request recommendations:

• Name your pull request your-development-type/short-description. Ex: feature/read-tiff-files
• Link to any relevant issue in the PR description. Ex: Resolves [Fix release #12], adds tiff file format support

Resolves #111

• Provide relevant tests for your feature or bug fix.
• Provide or update documentation for any feature added by your pull request.

Thanks for contributing!

[MatteBailey]

All right, so a couple things to note:

1. The tests should be passing, but there's an issue that is also affecting main right now causing them to fail.
2. I start PTO tomorrow, and ran out of time to update the docs. @JacksonMaxfield, sorry to be that guy, but any chance you could finish that off when you have time? The main think I can think of is noting in the dev docs that you should run git submodule update --init --recursive after checking out the project to make sure you also check out the submodule. I suppose we should update metadata/README.md as well.

[evamaxfield]

All right, so a couple things to note:

1. The tests should be passing, but there's an issue that is also affecting `main` right now causing them to fail.

2. I start PTO tomorrow, and ran out of time to update the docs. @JacksonMaxfield, sorry to be that guy, but any chance you could finish that off when you have time? The main think I can think of is noting in the dev docs that you should run `git submodule update --init --recursive` after checking out the project to make sure you also check out the submodule. I suppose we should update `metadata/README.md` as well.

Sounds good. Thanks for the update and have a wonderful break 🙂

[codecov-commenter]

Codecov Report

Merging #292 (3832c57) into main (a50da24) will increase coverage by 0.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #292      +/-   ##
==========================================
+ Coverage   95.09%   95.37%   +0.27%     
==========================================
  Files          35       35              
  Lines        2612     2617       +5     
==========================================
+ Hits         2484     2496      +12     
+ Misses        128      121       -7     

Impacted Files	Coverage Δ
aicsimageio/readers/czi_reader.py	95.43% <100.00%> (-0.27%)	⬇️
aicsimageio/tests/metadata/test_utils.py	100.00% <100.00%> (ø)
aicsimageio/tests/conftest.py	92.30% <0.00%> (-3.85%)	⬇️
aicsimageio/tests/readers/test_czi_reader.py	96.29% <0.00%> (-3.71%)	⬇️
aicsimageio/readers/ome_tiff_reader.py	96.94% <0.00%> (-0.77%)	⬇️
aicsimageio/metadata/utils.py	91.22% <0.00%> (+7.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a50da24...3832c57. Read the comment docs.

[MatteBailey]

@JacksonMaxfield @toloudis I finally got through all the feedback! Let me know if there's anything else.

[evamaxfield]

Ignoring the doc build failure because of git tag oddities, this all looks great. I have one comment which is just to add back html cov generation because I very commonly use it. But other than that, good to go!

[evamaxfield]

Oh, and lint is failing because of the OmeTiffWriter failure. Ignoring that as well.

[evamaxfield]

Going to wait for @toloudis, @AetherUnbound, @heeler, or @griffinfujioka before merge because this is a big one.

[toloudis]

Does anyone ever download the tarball? Does the tarball include the submodule stuff?

[evamaxfield]

I have no idea but this has just been a part of the cookiecutter since forever.

[MatteBailey]

@JacksonMaxfield thoughts on this? Not sure how the tarball gets generated.

[evamaxfield]

GitHub generates the tarball for us.

[toloudis]

This is all looking fine.

For the sake of discussion, I'm just going to say this feels like a premature submoduling, in the sense that the whole thing would be much cleaner and simpler if the XSLT lived in here closer to the CziReader.
Who are the other clients of the czi-to-ome-xslt? Is anyone waiting in line to use it?

[MatteBailey]

This is all looking fine.

For the sake of discussion, I'm just going to say this feels like a premature submoduling, in the sense that the whole thing would be much cleaner and simpler if the XSLT lived in here closer to the CziReader.
Who are the other clients of the czi-to-ome-xslt? Is anyone waiting in line to use it?

I don't disagree. I can definitely imagine it being useful to have the XSLT live in its own repo so it can easily be used in project using other languages, but I'm not sure if that is actually going to happen.

[AetherUnbound]

Looks great! Just one comment for the build.

I'll also add that I think this would probably be fine to live in the repo rather than as a submodule (since submodules can be a bit of a pain).

[evamaxfield]

I would generally agree that we have no idea if anyone will use the XSLT in any capacity outside of AICSImageIO however, part of the reason @heeler and I split it up was to have a different build system than building all of aicsimageio.

i.e. make the barrier to contribution for others as low as possible. The original goal was to have people on the Microscopy team work on the XSLT not us and setting up python + all the tooling in aicsimageio is harder than setting up 1 package in Python and running a script to see the conversion.

[MatteBailey]

I would generally agree that we have no idea if anyone will use the XSLT in any capacity outside of AICSImageIO however, part of the reason @heeler and I split it up was to have a different build system than building all of aicsimageio.

i.e. make the barrier to contribution for others as low as possible. The original goal was to have people on the Microscopy team work on the XSLT not us and setting up python + all the tooling in aicsimageio is harder than setting up 1 package in Python and running a script to see the conversion.

I think that's reasonable. Since the work is already done to integrate the submodule, I would be inclined to leave this as-is for now. We can always revisit if it becomes painful.