feat: get_obstore_store function for creating Obstore store with Open PC credentials#69
Conversation
…en PC credentials
planetary_computer/_obstore.py
Outdated
| "the optional dependency 'obstore'." | ||
| ) from e | ||
|
|
||
| def credential_provider() -> AzureSASToken: |
There was a problem hiding this comment.
To make this work with MPCPro, can you:
- Rename the inner function
credential_providertoplanetary_computer_credential_provider - Add an optional callable argument to
get_obstore_storethat returns anAzureSASToken. When it's None, use theplanetary_computer_credential_providerfunction. - Write a function in
_obstore.pythat returns callable credential providers for MPCPro instances namedplanetary_computer_pro_credential_provider.
Such that an end-user can do this:
from planetary_computer import get_obstore_store, planetary_computer_pro_credential_provider
from planetary_computer.utils import parse_blob_url
stac_item : pystac.Item = ... # results of querying a geocatalog
account, container = parse_blob_url(stac_item.assets["asset"].href)
store = get_obstore_store(account, container, credential_provider=planetary_computer_pro_credential_provider("https://geocatalog.azure.com")Maybe the call to parse_blob_url is not necessary and get_obstore_store can just accept a string argument and parse it inside? So that callers can just pass the asset href to get_obstore_store and not have to think about parsing account names and containers.
There was a problem hiding this comment.
Thinking about this a bit more:
While we can have a credential_provider parameter that matches obstore's own API, it seems a little verbose, and I think it's exposing concepts that are unnecessary here because we know users will always be using a credential provider. It's just a question of which one: Open PC or PC Pro, and that's all we need to have in our signature here. And I think we can figure out a simpler user API here in the context of users who will definitely be using PC.
There was a problem hiding this comment.
Maybe the call to parse_blob_url is not necessary and
get_obstore_storecan just accept a string argument and parse it inside? So that callers can just pass the asset href toget_obstore_storeand not have to think about parsing account names and containers.
This is why in obstore we have both AzureStore.__init__ and AzureStore.from_url so that we can customize the signature for the two different entry points.
There was a problem hiding this comment.
- rename to
get_obstore - Rename to credential provider to default to optional argument with default of open pc provider
- One day we can swap in the mpc pro credential provider there.
There was a problem hiding this comment.
@ghidalgo3 We can't have a default argument as an instance of the credential provider because the credential provider needs to know the account_name and container_name. So for now it defaults to None and that defaults to the open PC credential provider.
|
@microsoft-github-policy-service agree [company="Development Seed"] |
|
@microsoft-github-policy-service agree company="Development Seed" |
|
Can you please remove Python 3.8 from the test matrix? |
|
Obstore doesn't have wheels for 3.8 so this CI is failing on version 3.8. So we should remove the CI tests for 3.8 as well. |
|
I removed Python 3.8 from CI @ghidalgo3 Note that the minimum Python version for install is still 3.7: |
|
Can you please also increase the minimum python version in the toml to 3.9? |
|
I have edited the settings to allow your commits to run CI/CD automatically. |
|
I'm not sure it worked |
|
@ghidalgo3 looks like CI is finally passing! |
Obstore is an alternative to fsspec that's cleaner and can be more performant.
We might also want to consider whether there's an API that would allow to construct a store with either open PC or PC pro with a single function signature?
@ghidalgo3