Skip to content

http: migrate to fsspec & aiohttp #6525

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

Merged
merged 1 commit into from
Sep 7, 2021
Merged

Conversation

isidentical
Copy link
Contributor

@isidentical isidentical commented Sep 2, 2021

Resolves #6413

Differences compared to the old HTTPFileSystem (requests based):

  • We no longer have digest authentication since it is not supported by the aiohttp, at least not yet. (there is an old PR, ~3 years old that is still open). Instead of a deprecation period, now the config option auth with the value digest will raise an error saying that the option is not available. In the future, if this feature gets accepted to the aiohttp we could bring back the support, but it doesn't seem to be going to happen anytime soon.

@isidentical isidentical force-pushed the httpfs branch 2 times, most recently from 9d58b47 to c5b1926 Compare September 6, 2021 10:50
@isidentical isidentical marked this pull request as ready for review September 6, 2021 10:51
@isidentical isidentical requested a review from a team as a code owner September 6, 2021 10:51
@isidentical isidentical requested review from pared and efiop September 6, 2021 10:51
@efiop
Copy link
Contributor

efiop commented Sep 6, 2021

@isidentical Btw, since we are depending on fsspec's core more and more (e.g. that lexist issue you've mentioned today), let's setup testing for upstream fsspec? E.g. as a daily build.

@isidentical
Copy link
Contributor Author

Makes sense!

@efiop
Copy link
Contributor

efiop commented Sep 6, 2021

@isidentical Also, could you please elaborate in the PR description on the things we are missing here and other differences introduced by this? E.g. digest auth.

@skshetry
Copy link
Collaborator

skshetry commented Sep 6, 2021

+1 for httpx.

@efiop
Copy link
Contributor

efiop commented Sep 6, 2021

@skshetry could you elaborate, please?

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

LGTM, one question.

@efiop
Copy link
Contributor

efiop commented Sep 7, 2021

@isidentical Please clarify the path we will take about digest auth. I know that we've talked about it privately, but we need it to be visible for everyone :)

@isidentical
Copy link
Contributor Author

@efiop updated the PR description.

@efiop
Copy link
Contributor

efiop commented Sep 7, 2021

For the record: digest auth from our perception seems to be used rare enough that it makes sense for us to take the risk and to drop it for now. If there will be demand to bring it back, we will consider investing time in it. This is bad to drop a feature like that, but in this case it is worth it to move our filesystems/objects forward.

@isidentical isidentical merged commit 6b582bd into iterative:master Sep 7, 2021
@efiop efiop added the enhancement Enhances DVC label Sep 7, 2021
@jorgeorpinel
Copy link
Contributor

Hi. Do we know in what exact release (DVC version) this will be applied? For iterative/dvc.org#2802 (comment)

@efiop
Copy link
Contributor

efiop commented Sep 10, 2021

@jorgeorpinel 2.7.1

@jorgeorpinel
Copy link
Contributor

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhances DVC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http: migrate to fsspec
5 participants