search

spacetelescope / stdatamodels

https://stdatamodels.readthedocs.io
Other
5 stars 25 forks source link

Add asdf resources test and update schemas for new transform schemas #258

Closed braingram closed 6 months ago

braingram commented 8 months ago

This PR adds a test for "resources" (schemas, manifests, metaschemas) registered with asdf. The test checks:

There are a few issues with the resources that this test revealed:

===================================================================================================== short test summary info ======================================================================================================
FAILED test_integration.py::test_resource_id[resource19] - AssertionError: id[http://stsci.edu/schemas/jwst_datamodel/nrsmos_apcorr.schema] for resource[/Users/bgraham/projects/src/stdatamodels/src/stdatamodels/jwst/datamodels/schemas/nrsfs_apcorr.schema.yaml] did not return the co...
FAILED test_integration.py::test_resource_id[resource34] - AssertionError: id[http://stsci.edu/schemas/jwst_datamodel/abvegamag_offset.schema] for resource[/Users/bgraham/projects/src/stdatamodels/src/stdatamodels/jwst/datamodels/schemas/abvegaoffset.schema.yaml] was not registered...
FAILED test_integration.py::test_resource_id[resource82] - AssertionError: id[http://stsci.edu/schemas/jwst_datamodel/keyword_lampmode.schema] for resource[/Users/bgraham/projects/src/stdatamodels/src/stdatamodels/jwst/datamodels/schemas/keyword_lampstate.schema.yaml] did not retur...

These are all due to the "id" in the resource not matching the "uri" used to register the resource (although the errors differ if the "id" used matches a different "uri" as is the case for nrsfs_apcorr.schema.yaml and keyword_lampstate.schema.yaml). The "uri" is constructed based on the location of the file by the DirectoryResourceMapping used to register the resource. For the datamodels schemas: https://github.com/spacetelescope/stdatamodels/blob/b7c7bcd83b7a32d908e3f63c5dece313b3611f7a/src/stdatamodels/jwst/datamodels/integration.py#L34-L37 using nrsfs_apcorr.schema.yaml as an example. This schema will be registered using the uri http://stsci.edu/schemas/jwst_datamodel/nrsfs_apcorr.schema which does not match the id in the file (which uses nrsmos instead of nrsfs).

The issues fixed with this PR are somewhat innocuous as they would only cause problems if the fixed schemas contained $refs (which none do).

Checklist

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 75.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 64.92%. Comparing base (4d7c3a6) to head (a86fc17). Report is 14 commits behind head on main.

Files Patch % Lines
src/stdatamodels/fits_support.py 66.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #258 +/- ## ========================================== + Coverage 64.84% 64.92% +0.08% ========================================== Files 103 104 +1 Lines 5694 5713 +19 ========================================== + Hits 3692 3709 +17 - Misses 2002 2004 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

braingram commented 6 months ago

Regression tests: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1295/ show 4 expected errors from changes in https://github.com/spacetelescope/jwst/pull/8336 and no failures due to the changes in this PR.

braingram commented 6 months ago

Thanks for the review!

While giving it a last look I noticed the snell-1.1.0 tag in the new manifest was still using the old snell-1.0.0 schema. I pushed a commit to fix the version mismatch: https://github.com/spacetelescope/stdatamodels/pull/258/commits/a86fc1756b0efa8feff0010109b70c162a0a211d When the CI passes I'll merge this PR unless there are any objections.

braingram commented 6 months ago

As discussed on slack, I added a test that schema and tag versions match. To avoid delays or changes to this PR I added the test in a draft PR based off the changes of this PR: https://github.com/spacetelescope/stdatamodels/pull/286 Once the CI passes there I'll open it for review and merge this PR.