-
Notifications
You must be signed in to change notification settings - Fork 128
Remove unused pystac.validation import #1583
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
64331b0 to
201e29b
Compare
Python import times are noticeable during interactive use of CLI applications, so push the (near) single-use imports into the functions where they are used so the import cost is paid when calling the functions instead of when starting the CLI to print the help or do some other unrelated action. Timings on top of stac-utils#1583 inside a Python:3.10-bookworm container. Before this change: $ uv run hyperfine --warmup 3 "python3 -c 'import pystac'" Benchmark 1: python3 -c 'import pystac' Time (mean ± σ): 67.2 ms ± 1.6 ms [User: 58.6 ms, System: 8.6 ms] Range (min … max): 62.5 ms … 69.9 ms 45 runs After this change: $ uv run hyperfine --warmup 3 "python3 -c 'import pystac'" Benchmark 1: python3 -c 'import pystac' Time (mean ± σ): 59.9 ms ± 1.5 ms [User: 52.0 ms, System: 7.8 ms] Range (min … max): 56.9 ms … 64.2 ms 48 runs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1583 +/- ##
==========================================
- Coverage 92.38% 92.32% -0.06%
==========================================
Files 55 55
Lines 8375 8381 +6
Branches 965 966 +1
==========================================
+ Hits 7737 7738 +1
- Misses 453 457 +4
- Partials 185 186 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
As this is a breaking change, we should put it behind a __getattr__ with a warning, e.g. (this is untested, so might be a bit broken):
def __getattr__(name: str) -> Any:
if name == "validation":
import warnings
import pystac.validation
warnings.warn("pystac.validation will not be automatically imported to the package in pystac v2.0. Instead, import it directly: `import pystac.validation`")
return pystac.validation
else:
raise AttributeError(f"module {__name__} has no attribute {name}")That way, we don't break any usage that looks like:
import pystac
pystac.validation.validate(...)This takes 23 000us to import on a Ryzen 9950X3D, and the import does not seem to be used after PR stac-utils#591 was merged.
201e29b to
e56d019
Compare
Your code worked! I added a DeprecationWarning/stacklevel, and some quotes to the AttributeError message to align with the default message. |
gadomski
left a comment
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.
I haven't figure out how to add a pytest test for this, since pystac.validation ends up in the global attribute dictionary thanks to fixtures, but this one-liner works as expected:
$ python -c "import pystac; item = pystac.read_file('tests/data-files/item/sample-item.json'); pystac.validation.validate(item)"
<string>:1: DeprecationWarning: pystac.validation will not be automatically imported to the package in pystac v2.0. Instead, import it directly: `import pystac.validation`This takes 23 000us to import on a Ryzen 9950X3D, and the import does not seem to be used after PR stac-utils#591 was merged. Co-authored-by: Pete Gadomski <[email protected]>
Related Issue(s):
Description:
This takes 23 000us to import on
a Ryzen 9950X3D, and the import
does not seem to be used after
PR #591 was merged.
Tested by making a file called
test.pycontaining:and then inside the Python 3.10 container running
uv run python3 ./test.pyfollowed byuv run python3 -X importtime ./test.py > timing.txt 2>&1a couple of times.Reasonable timing before change:
and a reasonable after:
PR Checklist:
pre-commit run --all-files)pytest)