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

DM-41688: Add Pydantic data model to felis #25

Merged
merged 2 commits into from Jan 10, 2024
Merged

Conversation

JeremyMcCormick
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick commented Nov 21, 2023

This PR adds a Pydantic v2 data model to felis describing the YAML schema format, as well as unit tests for the new module. A validate command was added to the CLI which will load any number of YAML files into Pydantic and print validation errors. This has been tested on all of the schemas in sdm_schemas/yml.

To run tests covering the new module:

cd tests && python -m unittest test_datamodel

To run validation on all of the SDM schemas using the CLI tool:

# Change to use your sdm_schemas location
felis validate ../sdm_schemas/yml/*.yaml

These changes make no attempt to replace usage of the existing "simple" model that is used throughout Felis. All existing functionality should be unaffected.

For more details, please see the message on this PR's single commit. Updates to the Felis documentation will be forthcoming.

@JeremyMcCormick JeremyMcCormick marked this pull request as draft November 21, 2023 18:29
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (d8b9e12) 98.12% compared to head (28f202c) 98.47%.
Report is 1 commits behind head on main.

Files Patch % Lines
tests/test_datamodel.py 99.39% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   98.12%   98.47%   +0.35%     
==========================================
  Files           6        7       +1     
  Lines         427      592     +165     
==========================================
+ Hits          419      583     +164     
- Misses          8        9       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JeremyMcCormick
Copy link
Collaborator Author

Sample output of new validation using Pydantic on sdm_schemas/yml/*.yaml:

validate.log

@timj
Copy link
Member

timj commented Nov 21, 2023

deescription? (at start of validate.log)

@JeremyMcCormick
Copy link
Collaborator Author

deescription? (at start of validate.log)

Someone misspelled "description" in the SDM schema itself, and the strict validation has caught the error.

Before this would not have been caught because extra attributes were allowed in the YAML files and they are disallowed now.

@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-41688 branch 3 times, most recently from aa5f06a to 6b2f62d Compare December 19, 2023 19:46
@JeremyMcCormick JeremyMcCormick marked this pull request as ready for review December 19, 2023 19:47
Copy link
Member

@timj timj 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 to me. Some minor comments.

python/felis/cli.py Outdated Show resolved Hide resolved
python/felis/datamodel.py Outdated Show resolved Hide resolved
python/felis/datamodel.py Outdated Show resolved Hide resolved
python/felis/datamodel.py Outdated Show resolved Hide resolved
python/felis/datamodel.py Show resolved Hide resolved
python/felis/datamodel.py Outdated Show resolved Hide resolved
python/felis/datamodel.py Show resolved Hide resolved
python/felis/datamodel.py Outdated Show resolved Hide resolved
python/felis/datamodel.py Outdated Show resolved Hide resolved
tests/test_datamodel.py Show resolved Hide resolved
@JeremyMcCormick JeremyMcCormick marked this pull request as draft January 3, 2024 19:01
@JeremyMcCormick JeremyMcCormick removed the request for review from ktlim January 3, 2024 19:22
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-41688 branch 3 times, most recently from e7bb6b1 to 6c62c6e Compare January 3, 2024 20:25
@JeremyMcCormick JeremyMcCormick marked this pull request as ready for review January 3, 2024 20:27
Copy link

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

Looks good; I am just asking for a few edits to comments.

python/felis/datamodel.py Outdated Show resolved Hide resolved
python/felis/datamodel.py Outdated Show resolved Hide resolved
python/felis/datamodel.py Outdated Show resolved Hide resolved
python/felis/datamodel.py Outdated Show resolved Hide resolved
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-41688 branch 4 times, most recently from 49d9916 to 56f5cea Compare January 10, 2024 23:20
This update adds a Pydantic v2 data model in the datamodel module along
with unit tests for the new functionality. A new CLI command "validate"
supports loading a YAML file in the top-level Schema class and
validating it. Validation errors will result in a non-zero return code.
Any number of YAML files may be provided at the command line and
wildcards may be used to test all the files in a directory.

The data model explicitly defines fields for all allowed keys, such
that the usage of undefined keys, including misspellings or unknown
annotations, will result in validation errors. All optional annotations
currently found in the SDM Schemas YAML files are supported by the data
model. Units and IVOA UCDs are checked using modules from astropy.
Validation of the contents of other fields is initially loose, some
accepting any string value. It is forseen that additional validation
rules for fields and data model classes will be added in the future.
@gpdf gpdf self-requested a review January 10, 2024 23:39
Copy link

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

Good to go

@JeremyMcCormick JeremyMcCormick merged commit 72d3897 into main Jan 10, 2024
9 checks passed
@JeremyMcCormick JeremyMcCormick deleted the tickets/DM-41688 branch February 1, 2024 23:14
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.

None yet

3 participants