GeoRaster2.save_cloud_optimized: Remove unused var

[maralonso]

_raster_opener can leak memory, since it never closes the object.

[codecov-io]

Codecov Report

Merging #262 into master will increase coverage by 0.04%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage   90.56%   90.61%   +0.04%     
==========================================
  Files          38       38              
  Lines        5745     5742       -3     
==========================================
  Hits         5203     5203              
+ Misses        542      539       -3

Impacted Files	Coverage Δ
telluric/georaster.py	93.47% <100%> (+0.26%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ec9f4dd...2776902. Read the comment docs.

[AmitAronovitch]

_raster_opener can leak memory, since it never closes the object.

we need to solve that - do you have a fix? (this PR is good, but seem unrelated to raster_opener)

[arielze]

I agree with @AmitAronovitch we should solve the issue on _raster_opener and implement it as a context manager

[astrojuanlu]

we should solve the issue on _raster_opener and implement it as a context manager

Something like this?

class RasterOpener:
    def __init__(self, filename, *args, *, **kwargs):
        try:
            self.raster = rasterio.open(filename, *args, **kwargs)
        except (rasterio.errors.RasterioIOError, rasterio._err.CPLE_BaseError) as e:
            raise GeoRaster2IOError(e)

    def __enter__(self):
        return self.raster

    def __exit__(self, exc_type, exc_value, traceback):
        self.raster.close()

...

    def _raster_opener(cls, filename, *args, *, **kwargs):
        with rasterio.Env(**cls.get_gdal_env(filename)):
            return RasterOpener(filename, *args, **kwargs)

That way, I believe we can keep using raster = self._raster_opener(...) (like it's done in several parts of the code) while

with self._raster_opener(...) as raster:
    ...

closes the raster after opening it, I believe.

[AmitAronovitch]

I think if you just add @contextlib.contextmanager above the original function, and replace the "return" with a "yield" then it should work.

On the other hand I think my solution would enforce using as contextmenager and cause all usages as regular function to fail.

I am not sure if it is an advantage or disadvantage (that depends how we use it in the code - my implementation will force us to review our usage, and might cause issues if we do any sophisticated stuff with the raster-opener).

[AmitAronovitch]

BTW, sorry @maralonso - when I wrote my original response above - I did not notice that the original save_cloud_optimized implementation had actually called _raster_opener, so I just failed to see the connection between your comment above to the PR :woman_facepalming:

So - let us open a different PR for raster_opener and discuss the implementation there. This one is about changes in save_cloud_optimized - if we merge as is, then the local issue there will be solved, separately from the _raster_opener issue

[astrojuanlu]

On the other hand I think my solution would enforce using as contextmenager and cause all usages as regular function to fail.

Yep, I tried that and it makes code like this fail:

https://github.com/satellogic/telluric/blob/2e256de7321a6a4b45be27316154d75f2220aa74/telluric/georaster.py#L725-L728