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

Patch main-ci workflow #434

Merged
merged 9 commits into from
May 3, 2024
Merged

Patch main-ci workflow #434

merged 9 commits into from
May 3, 2024

Conversation

anngvu
Copy link
Collaborator

@anngvu anngvu commented Apr 26, 2024

What this does

  • Update to schematic version 24.4.1, which supposedly should be faster for template generation and will appear soon in DCA
    image

  • Better recognition of when to commit, easy fix for below instances (which happens 2-3% of the time?)
    image

    • Additional explanation: some changes, like edits to notes: and source: in the src files in modules are like silent mutations and do not lead to meaningful changes in the jsonld, json files
  • Remove the -rr flag to exactly match our DCA config https://github.com/Sage-Bionetworks/data_curator_config/blob/prod/NF-OSI/dca_config.json#L25

Notes for reviewers on interpreting results and followup investigation

Update 1

@allaway, it looks like generation works fine, something has changed with validation (compare with the tests from your recent PR #433 which used 24.2.1).

Error:
image

Looked a bit at the code around the indicated line... I will modify the mock manifests to understand what the expected behavior now when we specify a "num" rule.
https://github.com/Sage-Bionetworks/schematic/blob/c51af2d8419645fa04a13cc387e7811f1afe5b35/schematic/models/validate_attribute.py#L1025

Update 2

OK, I realized we need to remove the -rr flag now since we no longer want to restrict Great Expectations rules. Now the only issue is

error: '' is not of type 'array'
[['2', 'modelSystemName', "'' is not of type 'array'", '']

Copy link

github-actions bot commented Apr 26, 2024

Test Suite Report

(outdated run -- scroll down to next report 👇🏼 )

Template Generation

template result link
AnimalIndividualTemplate
BiospecimenTemplate 😄 template link
ClinicalAssayTemplate 😄 template link
EpigeneticsAssayTemplate 😄 template link
FlowCytometryTemplate 😄 template link
GenomicsAssayTemplate 😄 template link
GenomicsAssayTemplateExtended 😄 template link
HumanCohortTemplate 😄 template link
ImagingAssayTemplate 😄 template link
LightScatteringAssayTemplate 😄 template link
MethylationArrayTemplate 😄 template link
MRIAssayTemplate 😄 template link
PharmacokineticsAssayTemplate 😄 template link
PlateBasedReporterAssayTemplate 😄 template link
ProcessedAlignedReadsTemplate 😄 template link
ProcessedExpressionTemplate 😄 template link
ProcessedVariantCallsTemplate 😄 template link
ProteomicsAssayTemplate 😄 template link
ProtocolTemplate 😄 template link
RNASeqTemplate 😄 template link
ScRNASeqTemplate 😄 template link
UpdateMilestoneReport 😄 template link
WESTemplate 😄 template link
WGSTemplate 😄 template link

Manifest Validation

manifest result expectation
GenomicsAssayTemplate_0.csv Lists can be blank if attr not required using ‘list like’ rule
GenomicsAssayTemplate_1.csv Mixing blanks and regular list values works
GenomicsAssayTemplate_2.csv Conditional validation for attributes is currently not supported
ScRNASeqTemplate_0.csv Single list val works by using ‘list like’ rule
ScRNASeqTemplate_1.csv 😄 Fail because of missing data in required field libraryStrand

@anngvu anngvu force-pushed the patch/main-ci-workflow branch from e346cee to 7bb6837 Compare April 26, 2024 16:59
@anngvu anngvu force-pushed the patch/main-ci-workflow branch from 7bb6837 to f31c252 Compare April 26, 2024 20:06
Copy link

github-actions bot commented Apr 26, 2024

Test Suite Report 24.4.1

Template Generation

template result link
AnimalIndividualTemplate 😄 template link
BiospecimenTemplate 😄 template link
ClinicalAssayTemplate 😄 template link
EpigeneticsAssayTemplate 😄 template link
FlowCytometryTemplate 😄 template link
GenomicsAssayTemplate 😄 template link
GenomicsAssayTemplateExtended 😄 template link
HumanCohortTemplate 😄 template link
ImagingAssayTemplate 😄 template link
LightScatteringAssayTemplate 😄 template link
MethylationArrayTemplate 😄 template link
MRIAssayTemplate 😄 template link
PharmacokineticsAssayTemplate 😄 template link
PlateBasedReporterAssayTemplate 😄 template link
ProcessedAlignedReadsTemplate 😄 template link
ProcessedExpressionTemplate 😄 template link
ProcessedVariantCallsTemplate 😄 template link
ProteomicsAssayTemplate 😄 template link
ProtocolTemplate 😄 template link
RNASeqTemplate 😄 template link
ScRNASeqTemplate 😄 template link
UpdateMilestoneReport 😄 template link
WESTemplate 😄 template link
WGSTemplate 😄 template link

Manifest Validation

manifest result expectation
GenomicsAssayTemplate_0.csv Lists can be blank if attr not required using ‘list like’ rule
GenomicsAssayTemplate_1.csv Mixing blanks and regular list values works
GenomicsAssayTemplate_2.csv Conditional validation for attributes is currently not supported
GenomicsAssayTemplate_control.csv 😄 There should be no issue with this template.
ScRNASeqTemplate_0.csv Single list val works by using ‘list like’ rule
ScRNASeqTemplate_1.csv 😄 Fail because of missing data in required field libraryStrand

@anngvu anngvu requested review from allaway and cconrad8 April 26, 2024 21:06
@anngvu anngvu mentioned this pull request May 2, 2024
1 task
@anngvu
Copy link
Collaborator Author

anngvu commented May 2, 2024

Hey @allaway @cconrad8, I think we can merge this, just wanted you both to be aware that:

  • Some validation tests are failing in 24.4.1, so just expect to see this on future PRs for now and not be too concerned -- have communicated this issue to Amy during D&T meeting, so we just wait to see what they say/if there's a fix -- we might need a temp workaround, e.g. by modifying a validation rule, but that would be a separate PR anyway
  • Also, this should fix the more minor issue with empty commits

Copy link
Contributor

@allaway allaway left a comment

Choose a reason for hiding this comment

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

this LGTM, thank you @anngvu !

@allaway allaway merged commit 9985494 into main May 3, 2024
2 checks passed
@allaway allaway deleted the patch/main-ci-workflow branch May 3, 2024 13:03
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