Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add cache to get_schema, combine instance checks with validate #6

Conversation

braingram
Copy link

Here are a few optimizations.

  1. Cache the retrieved_schema (as you suggested)
  2. Combine the additional_validators with the new _Validate.iter_errors.

Combining the additional_validators with _Validate.iter_errors removes 1 walk of the tree during validation.

However, these changes don't appear to drastically speed up the test suite (it's still 63-65 seconds on my computer). This is possibly because the test files/trees are mostly small. For a random jwst asdf file I found in my downloads (jwst_nirspec_ifupost_0012.asdf) validation takes 964 ms without the changes and 599 ms with the changes in this PR.

The following tests run slowly with jsonschema 4.18:

  • test_load_schema (and other test_load_schema_* tests): For example, test_load_schema_with_full_tag takes ~2 seconds with 4.17 and ~3 seconds with 4.18. The difference is mostly in the call to validate in asdf.schema.check_schema and appears to be attributable to jsonschema and not to anything we're doing.
  • test_large_block_index: ~3 seconds with 4.17, ~4.5 seconds with 4.18. Looking at a profile of the test, in 4.17 ~1 second is spend in descend (in jsonschema, not the one in asdf.schema) whereas in 4.18 descend takes up ~2.8 seconds. line_profiler doesn't seem to play nice with the dynamic class generation and isn't providing meaningful results when I try to profile lines in descend.

I think this is far as I'm going to get on this today but I'll update if I get more time to look at it (and find anything useful).

@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2023

Codecov Report

❗ No coverage uploaded for pull request base (eslavich-no-patch-iter-errors@c24fa51). Click here to learn what that means.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@                       Coverage Diff                        @@
##             eslavich-no-patch-iter-errors       #6   +/-   ##
================================================================
  Coverage                                 ?   96.19%           
================================================================
  Files                                    ?       97           
  Lines                                    ?    12289           
  Branches                                 ?        0           
================================================================
  Hits                                     ?    11822           
  Misses                                   ?      467           
  Partials                                 ?        0           

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

Copy link
Owner

@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!

@eslavich eslavich marked this pull request as ready for review March 28, 2023 03:00
@eslavich eslavich merged commit b1b9ec9 into eslavich:eslavich-no-patch-iter-errors Mar 28, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants