Feat exaspim#481
Conversation
|
@cbrrrry, before I look closely at code, could you describe how you see this being used? My first reaction is that it makes me nervous to have a v1 to v2 upgrader living here in the metadata mapper repo. It seems cleaner to me to put in the effort to have the rigs produce v2 metadata directly. A counter-argument, though, is that the v1 metadata could be treated as 'extracted' metadata, as described here. If that's the case, though, we'll want you to create a model in the extractor repo to verify that the metadata coming off the rigs remains in the expected format. |
|
Interesting. I wasn't aware of the extractor repo. I could certainly try that. This is the start of the effort to get exaSPIM v2 aind-data-schema compliant. The acquisition.json (and via proxy, instrument.json) are produced on the rig in v1 format. I could ask Adam if he is open to a PR to update his exaSPIM acquisition software to output v2, but I know testing time on the exaSPIM rigs is scant/precious. I had previously implemented this upgrader logic (using the aind-metadata-upgrader) in the aind-exaspim-data-transformation package, but Jon suggested it should live here instead. |
|
To answer your first question @dougollerenshaw - I envision this being used once during the upload process to generate a complete suite of V2 metadata. |
Do you literally mean "once"? As in, this is a one-time script to update metadata for existing data to be schema 2.0 compliant? In that case, I'd argue that it doesn't belong here and probably belongs in Dan's upgrader repo instead. My understanding of the intent of this repo is to hold the code that will generate metadata for each experiment going forward. Admittedly this isn't explicitly called out in the readme, so maybe my definition is too narrow. I'll definitely defer to @dbirman here. |
dbirman
left a comment
dbirman
left a comment
There was a problem hiding this comment.
I don't think we should proceed with this PR, the base instrument/acquisition coming off the ExaSPIM rig were generated too long ago and at this point are basically un-upgradeable. We should properly upgrade the code that builds the instrument.json to V2, normalize names across the two files, and update the code that makes the acquisition.json to V2.
Using the upgrader here is just delaying that process and also risks pinning the upgrader version in a mapper that is difficult to update later. That almost guarantees that when we make changes to the upgrader later we will then be forced to run migrations against these "almost-V2-but-not-quite" ExaSPIM assets.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def sanitize_instrument(inst_data: dict) -> dict: |
There was a problem hiding this comment.
This code should be a part of aind-metadata-upgrader, not part of a mapper.
|
Thanks for your feedback on this process @dbirman and @dougollerenshaw. I will close this PR and will work on a the suitable replacement of upgrading the acquisition and instrument producing code on the rig software. Will coordinate with Adam about this. |
3 participants
This mapper contains logic to upgrade v1 acquisition and instrument.jsons to v2. ExaSPIM outputs instrument_config.yaml files, which are parsed to insert complete v1 instrument.json.