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

Ab/implement multi base custom reader #3

Merged
merged 23 commits into from
Jun 5, 2024

Conversation

abarciauskas-bgse
Copy link

@abarciauskas-bgse abarciauskas-bgse commented Jun 3, 2024

This PR:

A few questions / todos which have been turned into new issues:

@abarciauskas-bgse abarciauskas-bgse marked this pull request as ready for review June 4, 2024 19:19
empty_stac_reader._get_reader(asset_info)


@pytest.mark.skip(reason="Too slow.")
Copy link
Author

Choose a reason for hiding this comment

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

this is very slow for some reason, even though as far as I can tell the mock_rasterio_open is working as expected.

@@ -195,35 +194,32 @@ def tile(
scale = scale or 1

tms = self.supported_tms.get(tileMatrixSetId)
with rasterio.Env(**env):
Copy link
Author

Choose a reason for hiding this comment

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

I removed this context manager because I saw one was being used in AssetReader#tile#_reader with self.ctx but should we have it here as well since as you mentioned there are 3 levels of reader (STAC, item and asset)?

Copy link
Author

Choose a reason for hiding this comment

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

I reverted this change as I could see why different context manager blocks are needed since the env passed in the request could be different than the env passed in the asset_info, but I would be curious to know it's intentional to have multiple nested rasterio.Env context managers.

Copy link
Member

Choose a reason for hiding this comment

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

it's intentional to have an env for each reader, so we can set variable like GDAL_INGESTED_BYTES_AT_OPEN using specific number

@abarciauskas-bgse abarciauskas-bgse changed the title [DRAFT] Ab/implement multi base custom reader Ab/implement multi base custom reader Jun 4, 2024
@abarciauskas-bgse
Copy link
Author

@vincentsarago I believe I have addressed all comments, do you mind taking another look?

There are some outstanding issues mostly around testing and type checking that I listed in the description that I will turn into issues.

@vincentsarago vincentsarago self-requested a review June 5, 2024 15:40
@abarciauskas-bgse abarciauskas-bgse merged commit e8803a4 into main Jun 5, 2024
6 checks passed
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.

2 participants