validators: avoid urllib.request at import-time#1416
validators: avoid urllib.request at import-time#1416Julian merged 3 commits intopython-jsonschema:mainfrom
urllib.request at import-time#1416Conversation
f37765b to
48bf15c
Compare
|
These two commits are not really related the changes I wanted to propose. But they were required to get the CI to be green.
Did my best to resolve these... please let me know if I should make changes or drop these commits 🙈 |
noxfile.py
Outdated
| """ | ||
| session.install("pip-audit", installable) | ||
| session.run("python", "-m", "pip_audit") | ||
| session.run("python", "-m", "pip_audit", "-r", str(REQUIREMENTS["docs"])) |
There was a problem hiding this comment.
Thanks, yeah this is related to crate-py/rpds#178 (comment) -- here I think the fix is different a bit from just this.
This noxenv is meant to ensure "things jsonschema depends on do not have open CVEs." -- for that we need to run it not just on the docs requirements file but on each set of dependencies (i.e. each possible way of installing jsonschema), which is what installable represents here (basically it's a packaging extra).
So this isn't exactly right as-is, but my confusion from the comment I linked still leads me to not know what the right fix is here yet, as indeed pip isn't a dependency.
There was a problem hiding this comment.
Yeah this commit feels very "hacky" and not a permanent solution. I'm unsure how to fix it though. Should I drop this commit?
There was a problem hiding this comment.
Yes maybe that's best and I'll see if I can understand better what the intended fix really is from pip-audit.
|
(Thanks for also fixing the other 2 issues -- I cherry picked one, and left a comment on the other.) As for the core of this PR -- I'm OK merging this kind of thing though ideally I'd like a benchmark as well so that it doesn't regress. I have on my (neverending) backlog to set up asv at some point, or some other ongoing benchmarking run -- at very least perhaps we can add a benchmark to the already existing Basically I just want some reminder or baseline to track for imports, so open to ideas as well, but otherwise the change itself is fine with me. |
|
@Julian thanks for the quick review. Reason for this change from my end is I've been looking at optimising one step in my Zephyr RTOS firmware build which imports
Not sure if I can help with the setting up benchmarks pipeline, I'm no Python expert :) I've only seen that Twisted started using codspeed recently https://github.com/twisted/twisted/tree/trunk/benchmarks their codspeed's website say its free for open source projects. It seems to work well but some tests can be flaky. I've not seen/used airspeed velocity (asv) before. |
This change improves jsonschema import time by ~50ms. On my end calling: python -X importtime -c "import jsonschema" * Before: 234ms * After: 173ms
|
Yes understandable! I'm of course not asking you to help get a whole benchmarking setup before considering merging -- perhaps just adding a file to https://github.com/python-jsonschema/jsonschema/tree/main/jsonschema/benchmarks called If that's something you're comfortable trying great, if not we can worry about it later. |
|
Sounds reasonable to me :) I try and see if I can create it later tonight. |
48bf15c to
69d388c
Compare
69d388c to
2779b47
Compare
|
I based the script a lot on the test in pandas they use asv. Running Before: After: The result is easily skewed by heavy CPU activity. Running |
7886790 to
3440138
Compare
3440138 to
1f3616b
Compare
|
Thanks! |
|
@Julian Hi 👋Any chance to have this improvement released soon? Maybe a point release? Just thinking it doesn't hurt to ask :) |
|
Hey, have just pushed one out! |
The goal with this PR is to help reduce the time required to import the jsonschema module.
On my end calling:
python -X importtime -c "import jsonschema"importtime output (main)
importtime output (this PR)
📚 Documentation preview 📚: https://python-jsonschema--1416.org.readthedocs.build/en/1416/