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 cerberus validation #15

Merged
merged 8 commits into from
Sep 16, 2024
Merged

Conversation

HuangXiaoyan0106
Copy link
Contributor

Improve validation step

related ticket
https://d3b.atlassian.net/browse/D3B-804

Reasons
As more data types need to be included in the manifest template, it’s worthwhile to convert the all-in-one manifest into dedicated manifests for each sequencing experiment and define separate validation rules for each.

Main changes

  • The new validation step follows the updated manifest templates and also supports validating combined sequencing experiment manifests.
    dev shinyapp: https://d3b-rstudio-connect-public-prd.d3b.io/content/42c4e1ff-c94e-4915-892d-88f5f3d67a1a/
    (It will be released to the prd shinyapp once approved.)

  • The new validation using the Cerberus python library.
    As Allison suggested, we should consider using a standard Python validation library to handle increasingly complex rules.

  • Use the same validation rules as in the Shinyapp.
    Previously, we split the validation rules into two levels: data transfer and harmonization, with the latter implemented in the Shiny app. Data transfer rules have fewer requirements compared to the harmonization one. Now, we apply the harmonization rules at the beginning of the data transfer step, since if the files don’t meet these rules, we won’t be able to perform harmonization.

  • Print appropriate validation message.

    • Validation Passed
    • Validation Warnings
      • Validation results include only warning messages related to file size checking.
    • Validation Failed

Copy link
Contributor

@sickler-alex sickler-alex left a comment

Choose a reason for hiding this comment

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

I still want to review this more and run it a few times, but this is my initial comment.

Copy link
Contributor

@sickler-alex sickler-alex left a comment

Choose a reason for hiding this comment

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

This looks good, but we need to increase the version number. Since we're adding new requirements and it's not backwards compatible, I think it should be v3.0.0, but I'm welcome to discussing this further.

@sickler-alex
Copy link
Contributor

Also, we need to wait to release this until after I do https://d3b.atlassian.net/browse/D3B-870

d3b_dff_cli/version.py Outdated Show resolved Hide resolved
@HuangXiaoyan0106 HuangXiaoyan0106 merged commit 72387e4 into master Sep 16, 2024
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.

2 participants