Skip to content

Conversation

kylebarron
Copy link

@kylebarron kylebarron commented May 22, 2025

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

"the optional dependency 'obstore'."
) from e

def credential_provider() -> AzureSASToken:
Copy link
Collaborator

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:

  1. Rename the inner function credential_provider to planetary_computer_credential_provider
  2. Add an optional callable argument to get_obstore_store that returns an AzureSASToken. When it's None, use the planetary_computer_credential_provider function.
  3. Write a function in _obstore.py that returns callable credential providers for MPCPro instances named planetary_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.

Copy link
Author

@kylebarron kylebarron May 28, 2025

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.

Copy link
Author

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 to get_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.

Copy link
Author

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.

Copy link
Author

@kylebarron kylebarron Jun 4, 2025

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.

@kylebarron
Copy link
Author

@microsoft-github-policy-service agree [company="Development Seed"]

@kylebarron
Copy link
Author

@microsoft-github-policy-service agree company="Development Seed"

@ghidalgo3
Copy link
Collaborator

Can you please remove Python 3.8 from the test matrix?

@kylebarron
Copy link
Author

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.

@kylebarron
Copy link
Author

I removed Python 3.8 from CI @ghidalgo3

Note that the minimum Python version for install is still 3.7:

requires-python = ">=3.7"

@ghidalgo3
Copy link
Collaborator

Can you please also increase the minimum python version in the toml to 3.9?

@ghidalgo3
Copy link
Collaborator

I have edited the settings to allow your commits to run CI/CD automatically.

@kylebarron
Copy link
Author

I'm not sure it worked

@kylebarron
Copy link
Author

@ghidalgo3 looks like CI is finally passing!

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