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 reference to rasterio.path. #1255

Merged
merged 3 commits into from May 18, 2022
Merged

Conversation

SpacemanPaul
Copy link
Contributor

@SpacemanPaul SpacemanPaul commented May 18, 2022

Rasterio have expressed regret at publishing rasterio.path in their public API and intend to deprecate it. See #1246

@Kirill888 said this was trivial, and indeed it was. I can't shake the feeling that I'm missing something, but the tests pass.

@SpacemanPaul SpacemanPaul marked this pull request as ready for review May 18, 2022 02:52
@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #1255 (43a66a5) into develop (686a23a) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1255      +/-   ##
===========================================
- Coverage    93.61%   93.60%   -0.01%     
===========================================
  Files          129      129              
  Lines        13146    13145       -1     
===========================================
- Hits         12306    12305       -1     
  Misses         840      840              
Impacted Files Coverage Δ
datacube/storage/_rio.py 94.11% <100.00%> (-0.04%) ⬇️

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 686a23a...43a66a5. Read the comment docs.

@@ -165,8 +164,7 @@ def open(self) -> Iterator[GeoRasterReader]:

try:
_LOG.debug("opening %s", self.filename)
with rasterio.DatasetReader(rasterio.path.parse_path(str(self.filename)),
sharing=False) as src:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sharing=False is important for Dask use-case, can't just drop it. Pretty sure you can pass it on to .open. Keep in mind that .open does extra checks with respect to GDAL environment and AWS settings and in the past that caused significant slow-downs. rasterio promised to keep supporting direct access to DatasetReader, plan is to allow string inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added sharing=False to the .open call and everything seems to still work.

DatasetReader appears not accept string inputs as of 1.2.10.

@SpacemanPaul SpacemanPaul merged commit 67dde96 into develop May 18, 2022
@SpacemanPaul SpacemanPaul deleted the remove_rasterio_path branch May 18, 2022 04:44
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.

Don't use rasterio.path
2 participants