Skip to content
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

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

Merged
merged 4 commits into from Jul 7, 2022

Conversation

carderne
Copy link
Contributor

@carderne carderne commented Jul 4, 2022

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
Copy link
Contributor Author

carderne commented Jul 4, 2022

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

@gjoseph92
Copy link
Owner

We're constructing rasterio DatasetReader objects directly, instead of going through rio.open, like @Kirill888 mentioned in rasterio/rasterio#2365 (comment). 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 rasterio/rasterio#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
Copy link
Contributor Author

carderne commented Jul 5, 2022

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.

@carderne carderne marked this pull request as draft July 5, 2022 18:32
@gjoseph92
Copy link
Owner

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: microsoft/PlanetaryComputer#80 (comment).

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

@gjoseph92
Copy link
Owner

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: microsoft/PlanetaryComputer#80 (comment).

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
Copy link

👋 @gjoseph92

FYI 1.3 is going to be published today or tomorrow rasterio/rasterio#2310 (comment)

@carderne
Copy link
Contributor Author

carderne commented Jul 5, 2022

PR here with the rasterio constraint: #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).

@gjoseph92 gjoseph92 linked an issue Jul 5, 2022 that may be closed by this pull request
Still needs a `poetry lock` once 1.3.0 is released
@carderne carderne marked this pull request as ready for review July 5, 2022 22:06
@sgillies
Copy link

sgillies commented Jul 5, 2022

@carderne rasterio's got your back: rasterio/rasterio@5d5d946.

@gjoseph92
Copy link
Owner

Thanks so much @sgillies!

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

@carderne
Copy link
Contributor Author

carderne commented Jul 6, 2022

Open source saves the day!

More absurdity from python-poetry/poetry#5121. If I remove the `coiled` dependency, locking takes ~1min.
@gjoseph92 gjoseph92 merged commit a1be4f1 into gjoseph92:main Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rasterio parse_path deprecation
4 participants