-
Notifications
You must be signed in to change notification settings - Fork 456
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
Add HfFileSystemStreamFile #1967
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1967 +/- ##
==========================================
- Coverage 82.55% 82.20% -0.36%
==========================================
Files 66 66
Lines 8159 8187 +28
==========================================
- Hits 6736 6730 -6
- Misses 1423 1457 +34 ☔ View full report in Codecov by Sentry. |
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.
Thanks for opening the PR @lhoestq. Left some comments but mostly to be sure was is a implementation decision and what is required by fsspec. In general when there is a doubt I'd prefer to have an explicit comment in the code for the future ourselves (I feel fsspec
is quite opinionated on some things).
Also, would it be possible to update the Usage
part of this guide to showcase how to use HfFileSystemStreamFile
and why.
Thanks in advance!
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.
A comment from me :)
added annotations + a retry when streaming + close connection :) |
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.
Thanks! One nit.
EDIT:
Maybe let's also add a test.
Adding a test was useful 🙃 I found two bugs and fixed them |
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.
Thanks for iterating on this PR @lhoestq! Looks good to me except for a failing test. Can you have a look at it please? (or remove the new line if not relevant). Thanks in advance!
Thanks for fixing the test @lhoestq. Failing tests are now unrelated. Let's merge this :) |
This allows faster file streaming. This is useful for streaming WebDatasets for example