-
Notifications
You must be signed in to change notification settings - Fork 20
feat: get_obstore_store
function for creating Obstore store with Open PC credentials
#69
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
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this work with MPCPro, can you:
- Rename the inner function
credential_provider
toplanetary_computer_credential_provider
- Add an optional callable argument to
get_obstore_store
that returns anAzureSASToken
. When it's None, use theplanetary_computer_credential_provider
function. - Write a function in
_obstore.py
that 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toget_obstore_store
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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