Skip to content

DO NOT MERGE 2414 change cardinality of was informed by to 1 to n #2422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mbthornton-lbl
Copy link
Contributor

@mbthornton-lbl mbthornton-lbl commented Apr 16, 2025

This PR provides:

  • an update to basic_classes WorkflowExecution - adding multivalue: true to the slot_usage for was_informed_by
  • Update to valid examples to be correct
  • Update invalid examples so that they are not invalid due to was_informed_by
  • Added a new invalid example WorkflowExecution-was-invormed-by-not-a-list

This should not be merged until after the April 2025 release since this requires coordination with nmdc-server changes.

@mbthornton-lbl mbthornton-lbl linked an issue Apr 16, 2025 that may be closed by this pull request
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 53 out of 54 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • nmdc_schema/gold-to-mixs.sssom.tsv: Language not supported

Copy link

PR Preview Action v1.6.1

🚀 View preview at
https://microbiomedata.github.io/nmdc-schema/pr-preview/pr-2422/

Built to branch gh-pages at 2025-04-16 00:59 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@@ -17,7 +17,8 @@ has_output:
img_identifiers:
- img.taxon:x1
name: xxx
was_informed_by: nmdc:omprc-11-djb2d2
was_informed_by:
- nmdc:omprc-11-djb2d2
Copy link
Collaborator

@eecavanna eecavanna Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the number of spaces (4) used for indentation on the added line differs from the number (2) used for indentation elsewhere in the file. I recommend using the same number (2) as is already in use in the file.

I have the same comment about several of the example data files.

@mbthornton-lbl mbthornton-lbl marked this pull request as draft April 16, 2025 01:04
@eecavanna
Copy link
Collaborator

I don't understand why these files were deleted from the repo:

image

@eecavanna
Copy link
Collaborator

I recommend including at least one example data file in which there is more than one value in the was_informed_by list.

@eecavanna eecavanna requested a review from turbomam April 16, 2025 01:12
@eecavanna
Copy link
Collaborator

eecavanna commented Apr 16, 2025

If I am understanding this schema change correctly (that the was_informed_by field must now contain a list), a migrator will be required in order to update the existing was_informed_by values in the database.

@kheal
Copy link
Contributor

kheal commented Apr 16, 2025

@mbthornton-lbl Can we please add a description to was_informed_by in this PR?

@aclum
Copy link
Contributor

aclum commented Apr 17, 2025

@eecavanna those are derived files and shouldn't be included or else we end up with a bunch of merge conflicts.

@aclum aclum changed the title 2414 change cardinality of was informed by to 1 to n DO NOT MERGE 2414 change cardinality of was informed by to 1 to n Apr 17, 2025
@eecavanna
Copy link
Collaborator

Hi @aclum, it makes sense to me that they not be updated in this PR. The thing I'm confused about is why they are being deleted from the repo (as opposed to doing nothing) in this PR.

aclum added 2 commits April 17, 2025 16:06
I made was_informed_by globally multivalued so removing making it multivalued on WorkflowExecution
@aclum
Copy link
Contributor

aclum commented Apr 17, 2025

I think the were accidentally added either here or with a previous PR

@mbthornton-lbl
Copy link
Contributor Author

@eecavanna I am not sure how those files got deleted either :-/. I merged main back into this branch to restore them.

@mbthornton-lbl mbthornton-lbl requested a review from Copilot April 23, 2025 20:36
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the cardinality of the "was_informed_by" field from a single scalar to a list, aligning the YAML examples and invalid test cases with the new specification. Key changes include:

  • Converting scalar "was_informed_by" entries to list format across several YAML files.
  • Updating valid and invalid example files to reflect the new multi-value pattern.
  • Adding a new invalid example for a non-list "was_informed_by" value.

Reviewed Changes

Copilot reviewed 48 out of 49 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/data/invalid/MetaproteomicsAnalysis-invalid-missing-metap-cat.yaml Changed scalar "was_informed_by" to a list
src/data/invalid/MetagenomeSequencing-bad_id_mention.yaml Reformatted "was_informed_by" as a list
src/data/invalid/MetagenomeSequencing-bad_id.yaml Reformatted "was_informed_by" as a list
src/data/invalid/MetagenomeAssembly-invalid-qc-status-rules.yaml Reformatted "was_informed_by" as a list
src/data/invalid/MetabolomicsAnalysis-metab_quantified.yaml Reformatted "was_informed_by" as a list
src/data/invalid/MetabolomicsAnalysis-invalid_id-4.yaml Reformatted "was_informed_by" as a list
src/data/invalid/MetabolomicsAnalysis-invalid_id-3.yaml Reformatted "was_informed_by" as a list
src/data/invalid/MetabolomicsAnalysis-invalid_id-2.yaml Reformatted "was_informed_by" as a list
src/data/invalid/MetabolomicsAnalysis-invalid_id-1.yaml Reformatted "was_informed_by" as a list
src/data/invalid/MetabolomicsAnalysis-invalid_execution_resource_not_in_enum.yaml Reformatted "was_informed_by" as a list
src/data/invalid/MetabolomicsAnalysis-invalid-missing_execution_resource.yaml Reformatted "was_informed_by" as a list
src/data/invalid/MetabolomicsAnalysis-invalid-has-slot-used.yaml Reformatted "was_informed_by" as a list
src/data/invalid/Database-metatranscriptome_workflow-invalidDatabase.yaml Reformatted "was_informed_by" as a list
src/data/invalid/Database-mags-img_identifiers-exceeds-cardinality.yaml Reformatted "was_informed_by" as a list (in two spots)
src/data/invalid/Database-invalid_calibration_slot.yaml Reformatted "was_informed_by" as a list
src/data/invalid/Database-WorkflowExecutionActivity-invalid.yaml Reformatted "was_informed_by" as a list
src/data/invalid/Database-WorkflowExecution-was-informed-by-not-a-list.yaml Introduced as a new invalid example with "was_informed_by" not in list format
src/data/invalid/Database-ReadQcAnalysisActivity-invalid.yaml Reformatted "was_informed_by" as a list
src/data/invalid/Database-MetabolomicsAnalysis-no_metabolomics_category.yaml Reformatted "was_informed_by" as a list
Files not reviewed (1)
  • nmdc_schema/gold-to-mixs.sssom.tsv: Language not supported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change cardinality of was_informed_by to 1 to n
4 participants