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

Attempt to use only public jsonschema interfaces #1490

Closed

Conversation

eslavich
Copy link
Contributor

Just checking if this cuts the mustard with our downstream packages, stay tuned...

@eslavich eslavich requested a review from a team as a code owner March 19, 2023 04:51
@eslavich eslavich marked this pull request as draft March 19, 2023 04:51
@github-actions github-actions bot added this to the 2.15.0 milestone Mar 19, 2023
@eslavich eslavich force-pushed the eslavich-no-patch-iter-errors branch 2 times, most recently from 43031bd to 33f54ab Compare March 19, 2023 07:34
@WilliamJamieson
Copy link
Contributor

@eslavich this looks like it will work really well, and the changes look nice at first glance. However, I have not had a chance to look over the changes in detail.

Since you are still testing this could you remove the jsonschema upper pin? It would be nice to go ahead and fix that issue.

@eslavich
Copy link
Contributor Author

Since you are still testing this could you remove the jsonschema upper pin?

This PR doesn't actually fix our issues with the jsonschema alpha release, it just positions us better to do so. That'll be coming up in a subsequent PR.

I wouldn't recommend doing a detailed review of this one yet, I need to rework part of it for performance reasons...

@braingram
Copy link
Contributor

braingram commented Mar 21, 2023

I think the asdf-astropy (and possibly the dkist errors) will be fixed by:
asdf-format/asdf-standard#374
asdf-format/asdf-transform-schemas#88
I also opened a PR against your branch eslavich#5 with:

  • a minor change to improve performance during schema validation with 4.18
  • re-enabling jsonschema 4.18 as an allowable dependency, re-adding it to devdeps
  • adding a devdeps coverage job (to allow the per-jsonschema branches to be tested)

To summarize things so far, it looks like this PR should allow compatibility with jsonschema 4.18. However, the more strict schema parsing in this newer version means that asdf will be unable to validate using schema for the current released versions of:

  • asdf-transform-schema
  • asdf-standard

There are also the issues you opened in jsonschema that should hopefully be addressed before we merge these changes:
python-jsonschema/jsonschema#1062
python-jsonschema/jsonschema#1061
Specifically python-jsonschema/jsonschema#1061 should allow us to avoid overwriting the private _resolver attribute: https://github.com/eslavich/asdf/blob/eslavich-no-patch-iter-errors/asdf/schema.py#L656

We are also now depending on referencing and jsonschema_specifications (both new dependencies of jsonschema 4.18) and should likely add those to the pyproject.toml (I'm inclined to wait on this until a release version of jsonschema 4.18 to see what the lower pins are at that time).

@eslavich
Copy link
Contributor Author

I've been futzing with this tonight, trying to understand the consequences of the immutable Registry object. It seems like when jsonschema.Validator makes a lookup to descend into a referenced schema:

https://github.com/python-jsonschema/jsonschema/blob/main/jsonschema/validators.py#L404

it loses the work that has been done to load our schema. That includes YAML parsing in our retrieve callback as well as whatever the Registry.crawl() method does. I guess that implies we'd better put a cache decorator on our retrieve method?

@braingram
Copy link
Contributor

I worked up a minimal case for the registry and opened an issue with jsonschema:
python-jsonschema/jsonschema#1065
It sounds like the retrieved resources should be included. I'm not sure if this applies to the lookup during descend.

@braingram braingram added the development No backport required label Mar 27, 2023
@github-actions github-actions bot modified the milestones: 2.15.0, 3.0.0 Mar 27, 2023
add cache to get_schema, combine instance checks with validate
@eslavich
Copy link
Contributor Author

eslavich commented Aug 1, 2023

This has been superseded by #1591

@eslavich eslavich closed this Aug 1, 2023
@eslavich eslavich deleted the eslavich-no-patch-iter-errors branch August 1, 2023 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development No backport required Downstream CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants