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

deprecate ignore_version_mismatch #1819

Merged

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Aug 7, 2024

This PR deprecates ignore_version_mismatch. Since asdf 3.0.0 it has done nothing (it was only used for the legacy extension API which was removed in asdf 3.0.0).

There are a few downstream uses in stdatamodels mostly in tests. However there is one jwst test that fails due to a non-specific warning filter:
https://github.com/spacetelescope/jwst/blob/89f126a035751674f29a9027e2deae11b3e63fca/jwst/assign_wcs/tests/test_schemas.py#L57

Requires spacetelescope/stdatamodels#313
and likely spacetelescope/jwst#8693 (or a release of stdatamodels with the above PR)
before merging. I'm opening it for review now in case there is discussion about the warning message (changes to the message would require updating the jwst PR).

Closes #1812

Checklist:

  • pre-commit checks ran successfully
  • tests ran successfully
  • for a public change, a changelog entry was added
  • for a public change, documentation was updated
  • for any new features, unit tests were added

@braingram braingram force-pushed the deprecate_ignore_version_mismatch branch from d3ea83b to ced39e8 Compare August 7, 2024 15:59
@braingram braingram added this to the 3.X milestone Aug 7, 2024
@braingram braingram marked this pull request as ready for review August 8, 2024 12:36
@braingram braingram requested a review from a team as a code owner August 8, 2024 12:36
Copy link
Member

@zacharyburnett zacharyburnett left a comment

Choose a reason for hiding this comment

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

LGTM, one suggestion for a slightly more explicit warning message

asdf/_asdf.py Show resolved Hide resolved
asdf/_asdf.py Show resolved Hide resolved
asdf/_asdf.py Show resolved Hide resolved
pytest_asdf/plugin.py Show resolved Hide resolved
asdf/_tests/test_deprecated.py Show resolved Hide resolved
@braingram braingram force-pushed the deprecate_ignore_version_mismatch branch from 8366d39 to 7ab3183 Compare August 12, 2024 18:01
@braingram
Copy link
Contributor Author

Thanks for taking a look.

Unfortunately the jwst PR was merged :-/ That's my bad for not waiting to open that one. It includes a warning ignore that matches on the message currently used in this PR.
https://github.com/spacetelescope/jwst/pull/8693/files#diff-3f8cadc9ec467d860d9ac11aa3e65b4f126e2c44da2e9f45692ea9917ac58526R64

We could:

  • make another jwst PR with the updated warning message
  • merge this PR (once the new jwst PR is merged and the stdatamodels PR is approved and merged)

Or if you think the message is "good enough" we could go ahead with the existing message. We'd still have to wait for the stdatamodels PR so making another jwst PR doesn't seem like an issue. Would you like me to update the warning filter in jwst?

@zacharyburnett
Copy link
Member

Thanks for taking a look.

Unfortunately the jwst PR was merged :-/ That's my bad for not waiting to open that one. It includes a warning ignore that matches on the message currently used in this PR. https://github.com/spacetelescope/jwst/pull/8693/files#diff-3f8cadc9ec467d860d9ac11aa3e65b4f126e2c44da2e9f45692ea9917ac58526R64

We could:

  • make another jwst PR with the updated warning message
  • merge this PR (once the new jwst PR is merged and the stdatamodels PR is approved and merged)

Or if you think the message is "good enough" we could go ahead with the existing message. We'd still have to wait for the stdatamodels PR so making another jwst PR doesn't seem like an issue. Would you like me to update the warning filter in jwst?

nah, I think this message is good enough!

@braingram braingram force-pushed the deprecate_ignore_version_mismatch branch from 7ab3183 to 9103cb9 Compare August 20, 2024 14:43
@braingram braingram force-pushed the deprecate_ignore_version_mismatch branch from 9103cb9 to 0e7ce93 Compare August 20, 2024 17:02
@braingram braingram merged commit 1a8b04b into asdf-format:main Aug 20, 2024
45 of 47 checks passed
@braingram braingram deleted the deprecate_ignore_version_mismatch branch August 20, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate and remove the now unused ignore_version_mismatch
2 participants