search

spacetelescope / stdatamodels

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

drop use of json_id in walk_and_modify callbacks #244

Closed braingram closed 8 months ago

braingram commented 8 months ago

This PR removes uses of the optional json_id argument for callbacks passed to asdf.treeutil.walk_and_modify.

In fits_support this argument was unused and removal has no effect.

In schema_editor it is likely better to not use this argument (it's use might lead to bugs like in https://github.com/asdf-format/asdf/pull/1716).

For some extra context, json_id contains the string value under the key "id" of the most closely nested dictionary for the contained object processed in the callback. This is useful for draft 4 jsonschema so any reference fragments can be resolved using the uri stored in the "id" (see this schema for an example id). Keyword files do not follow draft 4 and do not contain an "id" with a uri. Within the schema_editor code, the top level keyword files are combined: https://github.com/spacetelescope/stdatamodels/blob/44a5691d56e001f5f1a9c54167481fb73be14813/src/stdatamodels/jwst/datamodels/schema_editor.py#L231-L242 before calling 'resolve_references' to resolve references to other keyword files: https://github.com/spacetelescope/stdatamodels/blob/44a5691d56e001f5f1a9c54167481fb73be14813/src/stdatamodels/jwst/datamodels/schema_editor.py#L244 using the directory as the 'uri' for resolving these references: https://github.com/spacetelescope/stdatamodels/blob/44a5691d56e001f5f1a9c54167481fb73be14813/src/stdatamodels/jwst/datamodels/schema_editor.py#L393-L399 so that references can be found relative to the directory containing the keyword json files. The presence of an "id" key would break this resolution if it did not contain the directory that contains the keyword json files.

Locally the test_schema_editor jwst regression tests pass. Regression test run: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1117/ shows only unrelated errors (due to new emicorr reference file and engdb failure, both of which are occurring on jwst main and the random "missing_msa_fail/nofail").

Checklist

codecov[bot] commented 8 months ago

Codecov Report

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

Comparison is base (44a5691) 64.82% compared to head (1ed3b38) 64.84%.

Files Patch % Lines
src/stdatamodels/jwst/datamodels/schema_editor.py 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #244 +/- ## ========================================== + Coverage 64.82% 64.84% +0.01% ========================================== Files 103 103 Lines 5695 5694 -1 ========================================== Hits 3692 3692 + Misses 2003 2002 -1 ```

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

braingram commented 8 months ago

Thanks for the review!