Skip to content

Utilize async credentials for async blob storage access.#132

Merged
TomAugspurger merged 1 commit into
fsspec:masterfrom
lossyrob:fix/async-credentials
Nov 18, 2020
Merged

Utilize async credentials for async blob storage access.#132
TomAugspurger merged 1 commit into
fsspec:masterfrom
lossyrob:fix/async-credentials

Conversation

@lossyrob

Copy link
Copy Markdown
Contributor

#131 s is caused by the need to use async credentials in the Aysnc clients of AzureBlobFileSystem (shoutout to @TomAugspurger for finding the fix).

This PR modifies the AzureBlobFileSystem utilize async credentials. This requires the creation of both async and synchronous credentials, as the AzureBlobFile utilizes synchronous clients and takes credentials from the parent filesystem.

I tested this against my own use case; however I didn't find a way to create a unit test for these changes as Azurelite doesn't support service principal authentication. If there is a way to test this that you'd like me to add, please let me know!

Fixes #131

@lossyrob lossyrob force-pushed the fix/async-credentials branch from 1a83ffb to 8f625d8 Compare November 17, 2020 00:38

@TomAugspurger TomAugspurger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me at a glance (other than the linting issue).

Do you have a sense for whether AzureBlobFile can be made to work with an async credential? It seems a bit strange to carry both around, and if we're forced to choose one then async is likely the way to go (@martindurant might have experience here from s3fs and gcsfs, though I don't know if they have async variants of auth).

Comment thread adlfs/spec.py
This commit modifies the _get_credential_from_service_principal method
to return a tuple which holds both an aysnc and sync version of
ClientSecretCredential. The async clients of the AzureBlobFileSystem
use the async credentials, and the AzureBlobFile utilizes the sync
credentials for its own synchronous clients.
@lossyrob lossyrob force-pushed the fix/async-credentials branch from 8f625d8 to 82cc012 Compare November 17, 2020 03:14
@lossyrob

Copy link
Copy Markdown
Contributor Author

Thanks for taking a look! Unclear why there's both async and sync - perhaps @hayesgb could say? I agree moving AzureBlobFile to async IO would be the move if possible; perhaps that can be recorded for a follow up issue if needed.

@martindurant

Copy link
Copy Markdown
Member

Neither s3fs nor gcsfs have async-specific auth, this is initially done at instantiation and blocks. It is possible for more than one reauth to be in progress at once, but this isn't a race to worry about, since each would return valid credentials.

@TomAugspurger

Copy link
Copy Markdown
Contributor

OK, thanks Martin.

@hayesgb any objections to merging and possibly consolidating the sync and async auth as a followup? I have write access here so I can merge, just don't want to step on toes since I haven't been active here for a while.

@hayesgb

hayesgb commented Nov 18, 2020

Copy link
Copy Markdown
Collaborator

Thanks guys. @TomAugspurger if you want to merge it in, feel free. If you don't have time, I'll do it, and add a release tomorrow.

And yes, the intent is to get the AzureBlobFile to async as well. The challenge I encountered is mixing sync and async operations from fsspec and the azure sdk, and the performance gains on the filesystem operations made it worth making this an intermediate step. Appreciate the help.

@TomAugspurger

Copy link
Copy Markdown
Contributor

Gotcha, thanks. This is all very new, but I suspect that we'll have more and more operations moving over to async (in fsspec) will hopefully make being all-async for auth here easier.

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.

AzureBlobFileSystem fails to open file with service principle authentication

4 participants