gjoseph92 / stackstac

Turn a STAC catalog into a dask-based xarray
https://stackstac.readthedocs.io
MIT License
238 stars 49 forks source link

Remove calls to rio.parse_path (maintain compat with rasterio==1.3) #155

Closed carderne closed 2 years ago

carderne commented 2 years ago

rasterio.path (and therefore, rasterio.parse_path) is being deprecated in rasterio 1.3 and removed in 1.4.

It is called internally anyway by DatasetBase so shouldn't be necessary to call it while initing.

Have checked that this works for my use case, YMMV.

carderne commented 2 years ago

NB: No formatting applied (to keep diff small), but happy to black if preferred.

gjoseph92 commented 2 years ago

We're constructing rasterio DatasetReader objects directly, instead of going through rio.open, like @Kirill888 mentioned in https://github.com/rasterio/rasterio/issues/2365#issuecomment-1086979284. That's why we were passing in parsed paths instead of raw strings. (It's also nice to not have to re-parse the path in every thread, but that should be a trivial amount of time.)

Based on https://github.com/rasterio/rasterio/pull/2428, it sounds to me like the changes in this PR (passing raw strings into the DatasetReader) will only work with rasterio>=1.3.0. So you'll need to update the requirement as well, or add some annoying version-conditional code that I'd rather not have.

carderne commented 2 years ago

Yeah sorry, should have marked this as a draft/discussiony PR.

In the meantime should probably lock down the rasterio dep, otherwise standard installs will break as soon as 1.3.0 lands.

diff --git a/pyproject.toml b/pyproject.toml
-rasterio = "^1.2.3"
+rasterio = "~1.2.3"

And then make the change from this PR + upgrade to ~1.3.0at the opportune moment.

gjoseph92 commented 2 years ago

Let's just make the rasterio change in this PR as well. It looks like someone who installs stackstac today and gets the new rasterio will have things fail: https://github.com/microsoft/PlanetaryComputer/discussions/80#discussioncomment-3084173.

This is quite an urgent fix. As soon as this is in, I'll cut a release.

gjoseph92 commented 2 years ago

Ah I see, rasterio 1.3.X is still in pre-release: https://pypi.org/project/rasterio/#history. I don't quite understand how a PC user ended up with a pre-release version then: https://github.com/microsoft/PlanetaryComputer/discussions/80#discussioncomment-3084173.

In that case, I agree. We should make and release this change right now:

diff --git a/pyproject.toml b/pyproject.toml
-rasterio = "^1.2.3"
+rasterio = "~1.2.3"

Then merge this PR and switch to rasterio = "^1.3.0" once it lands.

vincentsarago commented 2 years ago

👋 @gjoseph92

FYI 1.3 is going to be published today or tomorrow https://github.com/rasterio/rasterio/issues/2310#issuecomment-1175431663

carderne commented 2 years ago

PR here with the rasterio constraint: https://github.com/gjoseph92/stackstac/pull/157

Regarding people having rasterio 1.3bX installed: 1.2.X doesn't have wheels for Python 3.10 (which now ships as default on Ubuntu 22.04).

sgillies commented 2 years ago

@carderne rasterio's got your back: https://github.com/rasterio/rasterio/commit/5d5d946fbdc5a9babc7030e8f68b8ca4ccc538a2.

gjoseph92 commented 2 years ago

Thanks so much @sgillies!

Once 1.3.0 is out, we can merge this and release, but that takes the time pressure off.

carderne commented 2 years ago

Open source saves the day!