-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
Refs #36441 - Refactored GDAL functions to be lazily loaded. #19529
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
base: main
Are you sure you want to change the base?
Conversation
bd6a435
to
3b828dd
Compare
This comment was marked as resolved.
This comment was marked as resolved.
3b828dd
to
69ac875
Compare
self.assertEqual( | ||
gdal_version(), ("%s.%s.%s" % tuple(GDAL_VERSION)).encode() | ||
) |
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 think this formatting change can be reverted as it's not related to the PR
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.
Oh, it's more than a formatting change, it's also wrapping of GDAL_VERSION
with tuple
to force evaluation, since GDAL_VERSION
is now lazily evaluated:
def gdal_version_info():
ver = gdal_version()
m = re.match(rb"^(?P<major>\d+)\.(?P<minor>\d+)(?:\.(?P<subminor>\d+))?", ver)
if not m:
raise GDALException('Could not parse GDAL version string "%s"' % ver)
major, minor, subminor = m.groups()
return (int(major), int(minor), subminor and int(subminor))
- GDAL_VERSION = gdal_version_info()
+ GDAL_VERSION = lazy(gdal_version_info, tuple)()
It's been just long enough since I was deep in writing this PR, so I don't remember the nitty gritty details, but I remember running into issues with the test as previously written and lazy
. I can go back and do some digging to refresh my memory and get some more specifics around it if needed.
087290e
to
1e42bea
Compare
GDAL is now loaded only when first access rather than at import time, mirroring the approach taken for GEOS.
03bdfe6
to
f9a466f
Compare
This comment has been minimized.
This comment has been minimized.
Ah, I knew the feature freeze for 6.0 was happening soon, I guess I didn't realize it was today! Sorry for the pings Sarah and Jacob! Y'all got a lot on your plate getting ready for that and I hope I didn't bother too much. |
8862073
to
772c594
Compare
This comment has been minimized.
This comment has been minimized.
"tiger/line": "TIGER", | ||
} | ||
) | ||
if GDAL_VERSION[:2] <= (3, 10): |
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.
if GDAL_VERSION[:2] <= (3, 10): | |
# Accessing the lazy-loaded global GDAL_VERSION triggers a full load of the library. | |
if GDAL_VERSION[:2] <= (3, 10): |
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.
Suggesting a comment here because it's the reason why this is protected inside a separate method/property.
30f047a
to
bfed903
Compare
Co-authored-by: Benjamin Balder Bach <benjaoming@gmail.com>
07934d2
to
2ed1f5f
Compare
Trac ticket number
ticket-36441
Branch description
This PR implements lazy loading for the GDAL library in
django.contrib.gis.gdal
. The approach I took for this change was largely lifted from61d09e6
, but adapted for the slight differences between thedjango.contrib.gis.gdal
anddjango.contrib.gis.geos
modules.Changes:
lgdal
andlwingdal
variables now useSimpleLazyObject
to defer library loading until first call.GDALFuncFactory
class to wrap GDAL function calls with lazy loading behavior.django.contrib.gis.gdal.prototypes.generation
from functions to classes inheriting fromGDALFuncFactory
.argtypes
from positional to keyword argument for clarity. This is a slight behavioral change, asargtypes
is now always set (defaulting to[]
if not provided), matching theGEOSFuncFactory
pattern I used for inspiration. Previously, falsy argtypes values would leave the attribute unset.GDALFuncFactory
class-based API.utils_tests/test_lazy_libgdal.py
to verify lazy loading behavior and thatdjango.contrib.gis.gdal
can be imported without GDAL installed.Beyond the
argtypes
change, I tried my best to keep the code and logic within essentially the same. There were a few parts that I personally would have refactored for clarity while ultimately keeping the same output -- however, I resisted those urges and instead opted to copy the existing behavior and translate 1-to-1 to the new class-based API. (Well, except in the case ofargtypes
-- but that had prior art with the GEOS reference commit so I felt comfortable with that.)No documentation updates have been included.
API Compatibility
I'm unsure of what would be considered a "public" API in the
django.contrib.gis.gdal
module, since nothing is using the conventional_underscore
naming to show public/private intentions. However, if we base it purely on what's imported indjango/contrib/gis/gdal/__init__.py
, this does not alter that public API in any noticeable way. Only the imports related to the GDAL version information have been touched from that file, and the only modification has been to make them lazy instead of eager -- which should be transparent to users.The renaming of generation functions to classes could be considered backwards incompatible, but these appear to be internal to the
django.contrib.gis.gdal.prototypes
module. Happy to revert to snake_case naming if needed to keep the promise of API stability.Checklist
main
branch.I have attached screenshots in both light and dark modes for any UI changes.AI Assistance Disclosure (REQUIRED)
Note: Pulled this in from #19594 in anticipation of it being the template for PRs in the future and for full disclosure.
Please select exactly one of the following:
Required details for AI-assisted contributions
Tool(s) used: Claude Code w/ Sonnet 3.7 as the model, I think? It's been a bit so I'm not 100% sure.
What it was used for: Getting a general overview of the lazy GEOS commit
61d09e6
since this type of change is just enough out of my comfort zone. I did try to give it a chance to do the changes itself, but it was so hilariously off I abandoned the attempt and wrote it myself using the generated summary of the GEOS commit.I also had it generate a first draft of the laziness tests, but they were flawed and overly complicated (typical LLM stuff!). I ended up simplifying the tests myself, which you can see from the commit history of the
test_lazy_libgdal.py
file.I have manually reviewed and verified all AI-generated content.