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

remove uses of deprecated asdf.resolver #128

Merged

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Feb 27, 2023

asdf.resolver will be deprecated in 2.15: asdf-format/asdf#1362

A few uses were found in stdatamodels

  1. fits_support adds a resolver for a non-existent path:
    FITS_SCHEMA_URL_MAPPING = resolver.Resolver(
    [
    ('http://stsci.edu/schemas/fits-schema/',
    'file://' + META_SCHEMA_PATH + '/{url_suffix}.yaml')
    ] + resolver.DEFAULT_URL_MAPPING, 'url')
  2. jwst/datamodels/multiexposure provides resolvers that are equivalent to the defaults
    schema = asdf_schema.load_schema(
    self.schema_url,
    resolver=AsdfFile().resolver,
    resolve_references=True
    )
    core_schema = asdf_schema.load_schema(
    self.core_schema_url,
    resolver=AsdfFile().resolver,
    resolve_references=True
    )
  3. jwst/datamodels/schema_editor uses the default resolver to resolve schema references
    resolver = get_default_resolver()
    def resolve_refs(node, json_id):
    if json_id is None:
    json_id = url
    if isinstance(node, dict) and '$ref' in node:
    suburl = generic_io.resolve_uri(json_id, node['$ref'])
    parts = urlparse(suburl)
    fragment = parts.fragment
    if len(fragment):
    suburl_path = suburl[:-(len(fragment) + 1)]
    else:
    suburl_path = suburl
    suburl_path = resolver(suburl_path)
    try:
    subschema = aschema.load_schema(suburl_path,
    resolver,
    True)

1 and 2 are unnecessary and this PR removes these uses.

schema_editor duplicates some of the code internal to asdf see asdf.schema._load_schema_cached. The uses of resolver appear unnecessary as the call to generic_io.resolve_uri and schema.load_schema appear sufficient to resolve the schema references (as tested with the jwst regression test: https://github.com/spacetelescope/jwst/blob/master/jwst/regtest/test_schema_editor.py).

A complete run of the jwst regression tests with these changes will be needed. EDIT: jwst regression tests passed here: #128 (comment)

Resolves JP-nnnn
Resolves RCAL-nnnn

Closes #

This PR addresses ...

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

@braingram braingram added the no-changelog-entry-needed Trivial change that doesn't need an entry in the change log label Feb 27, 2023
@braingram braingram marked this pull request as draft February 27, 2023 18:47
@braingram
Copy link
Collaborator Author

JWST regression tests run against this branch passed without errors: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST/2243

JWST CI failure I believe is due to: #129

@braingram braingram force-pushed the asdf_resolver_deprecation branch from 2600217 to 454956c Compare February 27, 2023 22:23
@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (dec52f9) 65.17% compared to head (f4cb3a5) 65.17%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
- Coverage   65.17%   65.17%   -0.01%     
==========================================
  Files         101      101              
  Lines        5445     5438       -7     
==========================================
- Hits         3549     3544       -5     
+ Misses       1896     1894       -2     
Flag Coverage Δ
unit 65.17% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/stdatamodels/fits_support.py 90.28% <ø> (-0.09%) ⬇️
src/stdatamodels/jwst/datamodels/schema_editor.py 10.23% <ø> (-0.09%) ⬇️
src/stdatamodels/jwst/datamodels/multiexposure.py 57.89% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@braingram braingram marked this pull request as ready for review February 28, 2023 16:12
@braingram
Copy link
Collaborator Author

@tapastro @stscieisenhamer mentioned you may have run the schema_editor for the past few builds. Would you have time to look over the changes in this PR? Let me know if you have any questions.

@tapastro
Copy link
Collaborator

tapastro commented Mar 2, 2023

Sorry it took me a while to get back to you, @braingram! I don't know if there is good test coverage of the schema_editor, but I can try running it off this branch if you think this change may have affected functionality. To be honest, while I have used the code multiple times, the lines you've modified look completely unfamiliar and I'm not sure what would change on the face of it.

EDIT: I needed to read the whole post... I see the link to the test now. I'll review that and see if it covers the general usage.

@braingram
Copy link
Collaborator Author

braingram commented Mar 3, 2023

The new JWST CI failure is due to a version conflict (jwst requires stdatamodels <1.2, this repo is currently >=1.2 so the stpipe entry points fail to load with a VersionConflict):
https://github.com/spacetelescope/stdatamodels/actions/runs/4315086232/jobs/7528977635#step:12:848
A PR for stpipe is open that should fix this:
spacetelescope/stpipe#84

EDIT: given that the issue appears to be stpipe and the CI job runs with a pip installed stpipe this job is not expected to pass until a version of stpipe is released that has the fix mentioned above.

src/stdatamodels/jwst/datamodels/schema_editor.py Outdated Show resolved Hide resolved
@@ -394,8 +393,6 @@ def resolve_references(self, url):
"""
Resolve urls in the schema
"""
resolver = get_default_resolver()

def resolve_refs(node, json_id):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we can drop this whole method and replace with the asdf resolve_references=True flag? the try/catch that was added here seems like a kludge for some long ago schema URI problem that may not exist anymore.

If not, it's worth comparing this to the asdf implementation and updating the code here where it's fallen behind. It looks like a copy of an older version of the resolve_refs callback in asdf.schema.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's possible this isn't needed but given the lack of test coverage and that it sounds like this is a tool that's manually run I'm less inclined to make changes unless they're absolutely necessary. Perhaps @tapastro has some thoughts on this.

@braingram braingram force-pushed the asdf_resolver_deprecation branch from 8336173 to f6328df Compare March 8, 2023 13:13
@WilliamJamieson WilliamJamieson force-pushed the asdf_resolver_deprecation branch from f6328df to f4cb3a5 Compare March 8, 2023 14:40
@WilliamJamieson WilliamJamieson merged commit 2a418cb into spacetelescope:master Mar 8, 2023
@braingram braingram deleted the asdf_resolver_deprecation branch March 8, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog-entry-needed Trivial change that doesn't need an entry in the change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants