[Awaiting titiler release] Add reader opener options #90
Conversation
abarciauskas-bgse
changed the title
|
📚 Documentation preview will be available at: https://developmentseed.github.io/titiler-cmr/pr-previews/pr-90/ Status: ✅ Preview is ready! |
hrodmn
left a comment
hrodmn
left a comment
There was a problem hiding this comment.
I think this makes sense, but I have one question about the case in which s3_credentials is passed directly to e.g. CMRBackend.tile.
I am also wondering if:
We should not be passing s3_credentials as a keyword argument to xarray_open_dataset since this is a different signature from titiler.xarray.io.xarray_open_dataset. Perhaps titiler.xarray.io.xarray_open_dataset should accept kwargs which would make it easier to pass arbitrary arguments to xarray.open_dataset and thus more flexible for other applications to use titiler.xarray.io.xarray_open_dataset as-is?
I could see how updating titiler.xarray.io.xarray_open_dataset to accept kwargs would be useful, but would it know what to do with s3_credentials?
if opener and opener_options should be included in a lower-level class like BaseReader?
Maybe if the rasterio Reader were refactored to use an opener type of construct to open the source dataset that would make sense, but for now it is a strictly xarray feature so it probably needs to stay in the titiler.xarray.io.Reader only.
| else: | ||
| return self.reader_options | ||
|
|
||
| def _create_aws_session( |
There was a problem hiding this comment.
thanks for making these convenience functions - it makes the tile/part/etc functions easier to read!
titiler/cmr/backend.py
Outdated
| self, asset: Asset, kwargs: Dict[str, Any] | ||
| ) -> Optional[Dict]: | ||
| """Get s3 credentials from kwargs or via auth.""" | ||
| if "s3_credentials" in kwargs: |
There was a problem hiding this comment.
I think these kwargs come from **kwargs that are passed to functions like CMRBackend.tile() like tile - when would we be passing s3_credentials to the tile method in the kwargs? I thought those credentials were generated within the tile method, not before.
There was a problem hiding this comment.
This is something I'm going to continue to look into: If you recall, when we were pairing on this I was getting a JSON decode error from the earthaccess auth.get_s3_credentials(provider=provider) call, so I started passing the s3_credentials directly into the CMRBackend. However this is probably not the interface we want? So I'm trying to track down why we have the JSON decode error.
There was a problem hiding this comment.
I'm pretty confident the error that arises from auth.get_s3_credentials(provider=provider) is due to multi-threading in rio-tiler (and presumably that function not being entirely thread-safe). I don't think we should address it in this PR, but using a threading lock in aws_s3_credential seems to get around it.
Again, I'm not addressing that issue in this PR so I have just reverted the change which includes the conditional using s3_credentials in kwargs.
abarciauskas-bgse
changed the title
Note: This work must await the next release of titiler to incorporate
opener_options(see developmentseed/titiler#1248).This PR adds
opener_optionsin CMRBackend functions so that s3_credentials can be passed to titiler.cmr.reader.xarray_open_dataset.(Striking these questions as they are addressed by @hrodmn below)
I am also wondering if:We should not be passing s3_credentials as a keyword argument to xarray_open_dataset since this is a different signature from titiler.xarray.io.xarray_open_dataset. Perhaps titiler.xarray.io.xarray_open_dataset should accept kwargs which would make it easier to pass arbitrary arguments to xarray.open_dataset and thus more flexible for other applications to use titiler.xarray.io.xarray_open_dataset as-is?ifopenerandopener_optionsshould be included in a lower-level class likeBaseReader?