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.

Sign up for GitHub

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

Jump to bottom

Drop jsonschema #1614

Merged
merged 2 commits into from
Sep 18, 2023
Merged

Drop jsonschema #1614

merged 2 commits into from
Sep 18, 2023

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Aug 7, 2023 •
edited
Loading

@braingram braingram added this to the 3.0.0 milestone Aug 10, 2023
eslavich
eslavich reviewed Aug 14, 2023
View reviewed changes
asdf/exceptions.py Outdated
@@ -10,6 +10,9 @@
]


ValidationError.__module__ = "asdf.exceptions"
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that Python tools use this attribute to find the source code for the class, and setting it in this way may cause trouble. What do we gain by pretending that the module is asdf.exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Overwriting the module does break inspect.getsource (which appears to rely on getmodule).

I added this to fix the doctest to make it not contain a check for asdf._jsonschema.exceptions.ValidationError and instead check for asdf.exceptions.ValidationError and as an attempt to keep '_jsonschema' hidden. I don't think it's worth the breaking of inspect.getsource so I undid the change and updated the doctest.

CHANGES.rst Outdated
@@ -18,6 +18,7 @@ The ASDF Standard is at v1.6.0
- Convert numpy scalars to python types during yaml encoding
to handle NEP51 changes for numpy 2.0 [#1605]
- Vendorize jsonschema 4.17.3 [#1591]
- Drorp jsonschema as a dependency [#1614]
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here

Copy link
Contributor Author

@braingram braingram Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Speling is Hawrd :) Thanks! Fixed in: 616db20

@braingram braingram force-pushed the drop_jsonschema branch 2 times, most recently from 97bcc6d to 616db20 Compare August 15, 2023 16:13
@braingram braingram force-pushed the drop_jsonschema branch 2 times, most recently from 4de2929 to f0f95b7 Compare September 11, 2023 16:14
@braingram
Copy link
Contributor Author

Remaining jwst failures are unrelated see: spacetelescope/jwst#7874

@braingram braingram marked this pull request as ready for review September 13, 2023 14:13
@braingram braingram requested a review from a team as a code owner September 13, 2023 14:13
@zacharyburnett
Copy link
Member

zacharyburnett commented Sep 13, 2023 •
edited
Loading

would this also fix this issue in the stenv tests: https://github.com/spacetelescope/stenv/actions/runs/6162954331/job/16725854445#step:8:76

E   AttributeError: 'Namespace' object has no attribute 'jsonschema'

EDIT: After talking with @braingram we're going to try running the stenv asdf tests from source and see if that fixes the failures in the short term spacetelescope/stenv#116

@braingram
Copy link
Contributor Author

braingram commented Sep 13, 2023 •
edited
Loading

would this also fix this issue in the stenv tests: https://github.com/spacetelescope/stenv/actions/runs/6162954331/job/16725854445#step:8:76

E   AttributeError: 'Namespace' object has no attribute 'jsonschema'

EDIT: After talking with @braingram we're going to try running the stenv asdf tests from source and see if that fixes the failures in the short term spacetelescope/stenv#116

Thanks for following up about this. Doing some local testing if I run things with python 3.10 and pip the installed package tests run fine with pytest --pyargs asdf (installed from pypi). I'm only able to replicate the issue if I run things with conda. EDIT: I was able to replicate this with pip as well.

@braingram braingram mentioned this pull request Sep 13, 2023
Merged
eslavich
eslavich approved these changes Sep 18, 2023
View reviewed changes
Copy link
Contributor

@eslavich eslavich left a comment

Choose a reason for hiding this comment

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

LGTM

@braingram braingram merged commit 3719a8e into asdf-format:main Sep 18, 2023
41 of 42 checks passed
@braingram braingram deleted the drop_jsonschema branch September 18, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eslavich eslavich eslavich approved these changes

Assignees
No one assigned
Labels
Downstream CI
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@braingram @zacharyburnett @eslavich