Skip to content
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

Update structured_pattern in basic_classes #177

Merged
merged 5 commits into from
Jun 5, 2024

Conversation

kheal
Copy link

@kheal kheal commented May 22, 2024

Partially addresses: microbiomedata#1988 .

Checking that the use of the structured_pattern slot is appropriately defined on all classes that were refactored. This PR will also update all example data to comply.

When referencing a WorkflowExecution subclass it should be:

        structured_pattern:
          syntax: "{id_nmdc_prefix}:chcpr-{id_shoulder}-{id_blade}{id_version}$"

Otherwise it should be:

        structured_pattern:
          syntax: "{id_nmdc_prefix}:chcpr-{id_shoulder}-{id_blade}$"

@kheal
Copy link
Author

kheal commented May 22, 2024

@aclum and @turbomam . I've updated the id slot usage to better reflect the expected patterns and require the {id_version} for the WorkflowExecution subclasses. Tests are failing because I have not updated references of these classes in the exmaple data - there are 404 instances I'd need to fix (see below for summary). I could do that, but I'm worried about merge conflicts.

I'd rather we back merge nmcd_schema into berkeley_schema_fy24 first. Then I will update this branch and PR and fix the examples. Thoughts?

Screenshot 2024-05-22 at 4 45 52 PM

@@ -293,7 +293,7 @@ classes:
required: true
id:
structured_pattern:
syntax: "{id_nmdc_prefix}:calib-{id_shoulder}-{id_blade}{id_version}{id_locus}"
syntax: "{id_nmdc_prefix}:calib-{id_shoulder}-{id_blade}{id_version}$"
Copy link
Author

Choose a reason for hiding this comment

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

This should not have version, will fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@turbomam and I have talked about making this class abstract. Let's finalize the plan, if its abstract we can remove the structured_pattern.syntax.

Copy link
Author

Choose a reason for hiding this comment

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

I'd like to be part of those conversations. I don't quite see how we can make the CalibrationInformation abstract unless we subclass it, but maybe I'm missing something. I expect to instantiate instances of CalibrationInformation with the metabolomics metadata generation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, that was not a good place to leave that comment, I was referring about making WorkflowExecution abstract, not CalibrationInformation. This class seems like its not in the correct yaml, everything else in this yaml is for classes downstream of DataGeneration. Do you know the history on why this class is in this file and not nmdc.yaml?

Copy link
Author

@kheal kheal May 24, 2024

Choose a reason for hiding this comment

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

Oh yeah.....long story but the nmdc.yaml sources the workflow_execution.yaml but not vice versa. The CalibrationInformation is the range of the has_calibration slot which is on some of the WorkflowExecutions (until we merge in Berkeley and apply some ingestions). Once that has_calibration slot and range is fixed, we can move this class and associated slot definition to the nmdc.yaml.

WorkflowExecution is already abstract on berkeley fork, which makes total sense to me.

@@ -358,7 +358,7 @@ classes:
id:
required: true
structured_pattern:
syntax: "{id_nmdc_prefix}:wfch-{id_shoulder}-{id_blade}{id_version}{id_locus}"
syntax: "{id_nmdc_prefix}:wfch-{id_shoulder}-{id_blade}$"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mbthornton-lbl @scanon do we want versioning on the workflow chain records?

@aclum
Copy link
Collaborator

aclum commented May 24, 2024

Agree it makes sense to do this after a backmerge.

@kheal kheal self-assigned this May 28, 2024
@turbomam turbomam marked this pull request as ready for review June 5, 2024 22:17
@turbomam turbomam self-requested a review June 5, 2024 22:17
@turbomam turbomam merged commit a613c4f into main Jun 5, 2024
2 checks passed
@turbomam turbomam deleted the 1988_fix_structured_pattern branch June 5, 2024 22:55
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.

3 participants