-
Notifications
You must be signed in to change notification settings - Fork 0
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
Ab/implement multi base custom reader #3
Conversation
tests/test_asset_reader.py
Outdated
empty_stac_reader._get_reader(asset_info) | ||
|
||
|
||
@pytest.mark.skip(reason="Too slow.") |
There was a problem hiding this comment.
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.
titiler/stacapi/factory.py
Outdated
@@ -195,35 +194,32 @@ def tile( | |||
scale = scale or 1 | |||
|
|||
tms = self.supported_tms.get(tileMatrixSetId) | |||
with rasterio.Env(**env): |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@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. |
This PR:
A few questions / todos which have been turned into new issues:
bbox
andassets
ofinput
toAssetReader
#6