-
-
Notifications
You must be signed in to change notification settings - Fork 594
validators: avoid urllib.request
at import-time
#1416
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
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes maybe that's best and I'll see if I can understand better what the intended fix really is from pip-audit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed 👍
(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! |
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/