-
Notifications
You must be signed in to change notification settings - Fork 19
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
Open
kylebarron
wants to merge
7
commits into
microsoft:main
Choose a base branch
from
kylebarron:kyle/add-obstore-open-pc-integration
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4df68b5
feat: `get_obstore_store` function for creating Obstore store with Op…
kylebarron d5a6ef5
Install obstore in ci tests
kylebarron 22c97b5
Add default credential_provider argument
kylebarron 5afe151
Remove Python 3.8 from CI
kylebarron dd04899
lint
kylebarron 0b806ad
lint
kylebarron 59dea6f
try again to fix lint
kylebarron File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| from planetary_computer.sas import get_token | ||
|
|
||
| if TYPE_CHECKING: | ||
| import sys | ||
|
|
||
| from obstore.store import ( | ||
| AzureStore, | ||
| AzureConfig, | ||
| ClientConfig, | ||
| RetryConfig, | ||
| AzureSASToken, | ||
| ) | ||
|
|
||
| if sys.version_info >= (3, 11): | ||
| from typing import Unpack | ||
| else: | ||
| from typing_extensions import Unpack | ||
|
|
||
|
|
||
| def get_obstore_store( # type: ignore[misc] # overlap with kwargs | ||
| account_name: str, | ||
| container_name: str, | ||
| *, | ||
| prefix: str | None = None, | ||
| config: AzureConfig | None = None, | ||
| client_options: ClientConfig | None = None, | ||
| retry_config: RetryConfig | None = None, | ||
| **kwargs: Unpack[AzureConfig], # type: ignore # noqa: PGH003 (container_name key overlaps with positional arg) | ||
| ) -> AzureStore: | ||
| try: | ||
| import obstore | ||
| except ImportError as e: | ||
| raise ImportError( | ||
| "'planetary_computer.get_obstore_store' requires " | ||
| "the optional dependency 'obstore'." | ||
| ) from e | ||
|
|
||
| def credential_provider() -> AzureSASToken: | ||
| token = get_token(account_name, container_name) | ||
| return { | ||
| "sas_token": token.token, | ||
| "expires_at": token.expiry, | ||
| } | ||
|
|
||
| return obstore.store.AzureStore( | ||
| account_name=account_name, | ||
| container_name=container_name, | ||
| prefix=prefix, | ||
| config=config, | ||
| client_options=client_options, | ||
| retry_config=retry_config, | ||
| credential_provider=credential_provider, | ||
| **kwargs, # type: ignore # (container_name key overlaps with positional arg) | ||
| ) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
credential_providertoplanetary_computer_credential_providerget_obstore_storethat returns anAzureSASToken. When it's None, use theplanetary_computer_credential_providerfunction._obstore.pythat returns callable credential providers for MPCPro instances namedplanetary_computer_pro_credential_provider.Such that an end-user can do this:
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.Uh oh!
There was an error while loading. Please reload this page.
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_providerparameter 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.
This is why in obstore we have both
AzureStore.__init__andAzureStore.from_urlso 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.
get_obstoreUh oh!
There was an error while loading. Please reload this page.
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_nameandcontainer_name. So for now it defaults toNoneand that defaults to the open PC credential provider.