enable gdal.UseExceptions throughout pygeoprocessing #392
Conversation
| @@ -5,7 +5,6 @@ | |||
|
|
|||
| from . import geoprocessing | |||
| from .geoprocessing import DEFAULT_GTIFF_CREATION_TUPLE_OPTIONS | |||
| from osgeo import gdal | |||
| target_raster_path, int(local_n_cols), int(local_n_rows), 1, | ||
| gdal.GDT_Byte, options=GTIFF_CREATION_OPTIONS) | ||
|
|
||
| raster.SetSpatialRef(flow_dir_srs) |
There was a problem hiding this comment.
It seems like SetSpatialRef is equivalent to SetProjection but takes in a spatial reference object rather than a WKT string. Calling ExportToWKT on an empty spatial reference raises an unhelpful error, so I changed this to avoid having to get the WKT string at all.
There was a problem hiding this comment.
This looks great, @emlys thanks so much for going through everything so carefully!
My one request here is that we add docstrings to the decorator and the context manager so that they can be referred to more easily in the API documentation.
And I had one additional question that didn't fit in the line-by-line review: do we need to change anything with our minimum GDAL version stated in requirements.txt? All the tests are passing so we should at least be good to GDAL 3.2.2, but I just wanted to make sure that our current minimum GDAL requirement holds true.
Thanks!
| class GDALUseExceptions(object): | ||
|
|
||
| def __init__(self): | ||
| pass | ||
|
|
||
| def __enter__(self): | ||
| self.currentUseExceptions = gdal.GetUseExceptions() | ||
| gdal.UseExceptions() | ||
|
|
||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||
| if self.currentUseExceptions == 0: | ||
| gdal.DontUseExceptions() | ||
|
|
||
|
|
||
| def gdal_use_exceptions(func): | ||
| def wrapper(*args, **kwargs): | ||
| with GDALUseExceptions(): | ||
| return func(*args, **kwargs) | ||
| return wrapper |
There was a problem hiding this comment.
Since these two callables are at the top level, would it be worth adding a docstring to indicate their purpose?
|
@phargogh I added those docstrings, and I tested locally with gdal 3.0.4, our minimum supported version. All tests passed but it's worth noting that GDAL's error messages vary across versions and operating systems, hence some of the tests allowing multiple error types/messages, but they seem to be generally improving. So users of the oldest GDAL versions might encounter less helpful errors. |
|
I also had to remove a deprecated import and pin numpy due to some new dependency releases. |
There was a problem hiding this comment.
Thanks @emlys ! Everything looks great here w/r/t gdal.UseExceptions. I just remembered: since we are changing behavior here (changes in exceptions raised especially), should there be a note about it in HISTORY? Aside from that, I think this is ready to be merged!
|
@phargogh I added a note in history and also removed the numpy < 2 pin, since your PR is now merged |
Fixes #391
This PR adds a context manager called
GDALUseExceptionsthat turns ongdal.UseExceptionswithin the context and restores the original state afterward. Also adds a decorator that applies the context manager to a function.I first tried to use the context manager GDAL provides,
gdal.ExceptionMgr, but it didn't do what I needed. It doesn't silence theFutureWarning, and I noticed it behaved weirdly and didn't seem to always restore state correctly.I applied either the decorator or the context manager everywhere that we use GDAL, to each function individually so that our
gdal.UseExceptionsstate doesn't affect, and isn't affected by, other code. Since we aren't importing any libraries that might also change the state, I think it's guaranteed that exceptions will be enabled where we expect. I used the decorator as much as possible, and the context manager in a few places where cython or multiprocessing complained about the decorator.I removed some error handling that's made redundant by GDAL's exceptions. We have some tests that tested that error handling, and I thought about removing those tests, but decided to update them to test the GDAL exceptions instead. Just to make sure those show up right.
Also replaced all
assertTrue(x in y)withassertIn(x, y)because they were driving me crazy 🙃