Issue 6598: load_dataset broken for data_files on s3 #6862
Conversation
matstrand
force-pushed
the
issue-6598-load-dataset-broken-s3
branch
4 times, most recently
from
bf3c8c2
to
88cc5a3
Compare
matstrand
force-pushed
the
issue-6598-load-dataset-broken-s3
branch
from
c3c0c06
to
ded2cac
Compare
src/datasets/utils/file_utils.py
Outdated
| if scheme in ("s3", "s3a") and storage_options is not None and "hf" in storage_options: | ||
| # Issue 6598: **storage_options is passed to botocore.session.Session() | ||
| # and must not contain keys that become invalid kwargs. | ||
| del storage_options["hf"] |
There was a problem hiding this comment.
it would be great to have a more general solution for any filesystem than hardcoding something for S3
maybe in _prepare_single_hop_path_and_storage_options() in file_utils.py ?
There was a problem hiding this comment.
Thanks ! Can you please explain a bit more what a better general solution could be? Is this an option ? Move this logic to set the hf token to this protocol check Link
There was a problem hiding this comment.
Thanks both, seems the root issue is settingDownloadConfig.storage_options['hf'] in all situations. We may be able to remove that as test_load.py passes without it. It was added at #6028 but seems possibly obsoleted by #6784.
I've pushed a change. @lhoestq can you please trigger the CI so we can see if this introduces any regressions?
|
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. |
| @@ -93,7 +93,7 @@ def __post_init__(self, use_auth_token): | |||
| FutureWarning, | |||
| ) | |||
| self.token = use_auth_token | |||
| if "hf" not in self.storage_options: | |||
| if self.token is not None and "hf" not in self.storage_options: | |||
There was a problem hiding this comment.
I was worried that removing this altogether might break functionality for someone. This still might break CI but let's see before spending more time on it.
There was a problem hiding this comment.
you might have to revert this, since we need the endpoint to be in the storage options even if token is None
There was a problem hiding this comment.
I'm trying to account for that here. This way _prepare_path_and_storage_options ensures that the endpoint is in the storage options without needing to populate it for all protocols in __post_init__.
Fixes /issues/6598
I've added a new test case and a solution. Before applying the solution the test case was failing with the same error described in the linked issue.
MRE: