Update FunctionalAnnotationAggMember class for compatibility with MetaP Aggregation tables and implement migrator

[kheal]

This was moved from the berkeley fork to this repo. Original PR found here: microbiomedata#242

Description

This PR will

1. Modify the existing FunctionalAnnotationAggMember class to be able to accomodate both annotations from metagenomics and metaproteomics analyses. This is accomplished by exchanging the slot metagenome_anlaysis_id with the existing slot was_generated_by.
2. Implement a migrator to adjust existing records.
3. Deprecate the metagenome_anlaysis_id slot as there are now no classes using this slot.
4. Update tests accordingly
5. Add description to count slot
6. Add a unique_keys to FunctionalAnnotationAggMember class that is a combination of was_generated_byandgene_function_id See Add unique_keys to FunctionalAnnotationAggMember class #2155 for why this was removed from this PR.

This PR will close 1253

PR Information

What type of PR is this? (check all applicable)

• Schema change: Structure and content
  • deleted a slot

Related Issues

• Fixes: 1253

Did you add/update any tests?

• Yes
  • All example yamls are updated (including invalid examples
  • Doctest for migrator added
  • Added invalid example for FunctionalAnnotationAggMember to check for rule on count slot

Could this schema change make it so any valid data becomes invalid?

• Yes (A migrator is required)

If you answered "Yes", does this PR branch include that migrator?

• Yes

Does this PR have any downstream implications?

• Yes (Explain below)
  We will need to update the aggregation scripts (issue filed here: Update generate_metap_agg.py script to source ids from new slot nmdc-aggregator#13) and the data portal to use the new slot

[github-actions]

PR Preview Action v1.4.8
🚀 Deployed preview to https://microbiomedata.github.io/nmdc-schema/pr-preview/pr-2203/
on branch gh-pages at 2024-11-08 17:28 UTC

[shreddd]

Prior to merging / deploying this, I wanted to make sure that the needed changes also get rolled into
https://github.com/microbiomedata/nmdc-aggregator/
Otherwise this will break when a new collection is generated from the aggregator.

Possibly related? microbiomedata/nmdc-aggregator#24

[kheal]

Prior to merging / deploying this, I wanted to make sure that the needed changes also get rolled into https://github.com/microbiomedata/nmdc-aggregator/ Otherwise this will break when a new collection is generated from the aggregator.

Possibly related? microbiomedata/nmdc-aggregator#24

@shreddd - Yes that is the companion issue to keep the aggregator in step with this change.

[kheal]

@eecavanna or @brynnz22 - Do either of you have guidance for how the migrator should be named? It is currently nmdc_schema/migrators/migrator_from_XX_to_PR2203.py. I can either leave it as that or update, but otherwise I think this MR is about ready to be merged in.

[brynnz22]

Maybe migrator_from_11_0_3_to_11_1_0. I'm not sure what the next version of the schema will be. @turbomam do you have an ideas what the next version of the schema will be after merging this in?

[eecavanna]

Sorry about the delay! In a "workshop" all day today and Wednesday.

I agree with @brynnz22's recommendation. The "from" version is the currently released schema version. The "to" version is still TBD. The value @brynnz22 suggested makes sense to me as a placeholder (sufficient for merging, in my opinion, given that we don't know what the next schema version will be).

---

The requirement of putting the "to" version in the migrator puts us in this chicken-and-egg situation. I do plan to change this part of the procedure "soon" ™.

[brynnz22]

@kheal @eecavanna Since I am working on a migrator right now for the has_calibration slot, it sounds like this should be a partial migrator and same with the one I am working on. Because they will both probably feed into the same schema update. Does this seem right? My PR will be ready sometime today.

[kheal]

@brynnz22 that seems reasonable. #2204 also has a migrator that could be bundled into this if we decide to go down that route.

[kheal]

Updated the migrator to be a partial for the 11.1 release.

[kheal]

@turbomam - do you want to review this before we merge it in?

[turbomam]

@turbomam - do you want to review this before we merge it in?

yes, thanks. looking now.

[turbomam]

<3

[turbomam]

I would like to move that definition for count to slot_usage. The count slot could easily be used with different meanings in other class contexts.

[turbomam]

@sierra-moxon and @turbomam Could you please check that I've implemented the use of unique_keys correctly?

@kheal If you have tests that show valid and invalid re-use of either of the identifying slots, then I approve for merging.

Update: I get the sense that illegally duplicated id combinations aren't tested yet.

[turbomam]

This will be a great PR with a few tweaks. Let me know if you think any of my requests will be difficult (or harmful!)

[turbomam]

I would like to move that definition for count to slot_usage. The count slot could easily be used with different meanings in other class contexts.

[kheal]

@turbomam. Thanks for the input.

@turbomam or @sierra-moxon I could use some help with the test and/or the unique_keys slot. I added a test in the problem/invalid folder - it is passing validation but it shouldn't, so something is wrong, but I can't tell if it's the testing or the actual definition in the schema. I used the Isotope example here as guidance for how to use the unique_keys slot.

I've moved the description for the count slot to the class's slot_usage.

[kheal]

After consulting with @sierra-moxon, we've determined that uniqueness across objects is not supported in JSONSchema, and because linkml-validate uses the jsonschema serialization of the model to do validation, it is not checking the unique keys constraint. See #2155 for more discussion

Because of the lack of verifiability on the unique_keys constraint currently supported, I've removed that from this PR. Thank you @turbomam for encouraging adding a testing file for this where we uncovered this.