5537 speedup load
MEMBER
Awesome ! cc @mariosasko :)
2023-04-04T09:47:32Z
https://github.com/huggingface/datasets/pull/5704#issuecomment-1495669985
https://api.github.com/repos/huggingface/datasets/issues/5704
IC_kwDODunzps5ZJhzh
https://api.github.com/repos/huggingface/datasets/issues/comments/1495669985/reactions
2023-04-04T09:47:32Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1495669985
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
NONE
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/datasets/pr_5704). All of your documentation changes will be reflected on that endpoint.
2023-04-04T09:51:14Z
https://github.com/huggingface/datasets/pull/5704#issuecomment-1495674943
https://api.github.com/repos/huggingface/datasets/issues/5704
IC_kwDODunzps5ZJjA_
https://api.github.com/repos/huggingface/datasets/issues/comments/1495674943/reactions
2023-04-04T09:51:14Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1495674943
https://avatars.githubusercontent.com/u/99929124?v=4
https://api.github.com/users/HuggingFaceDocBuilderDev/events{/privacy}
https://api.github.com/users/HuggingFaceDocBuilderDev/followers
https://api.github.com/users/HuggingFaceDocBuilderDev/following{/other_user}
https://api.github.com/users/HuggingFaceDocBuilderDev/gists{/gist_id}
https://github.com/HuggingFaceDocBuilderDev
HuggingFaceDocBuilderDev
U_kgDOBfTMJA
https://api.github.com/users/HuggingFaceDocBuilderDev/orgs
https://api.github.com/users/HuggingFaceDocBuilderDev/received_events
https://api.github.com/users/HuggingFaceDocBuilderDev/repos
https://api.github.com/users/HuggingFaceDocBuilderDev/starred{/owner}{/repo}
https://api.github.com/users/HuggingFaceDocBuilderDev/subscriptions
User
https://api.github.com/users/HuggingFaceDocBuilderDev
public
COLLABORATOR
Hi, thanks for working on this!

Your solution only works if the `root` is `""`, e.g., this would yield an incorrect result:
```python
dset = load_dataset("user/hf-dataset-repo", data_dir="path/to/data_dir")
```

Also, the `HfFileSystem` implementation in `datasets` will be replaced with the more powerful [one](https://github.com/huggingface/huggingface_hub/blob/main/src/huggingface_hub/hf_file_system.py) from `huggingface_hub` soon (I plan to open a PR that makes `find` much faster in the coming days). 

So I don't think we want to merge this PR in the current state, but thanks again for the effort.

 (I'll comment on the original issue to propose a cleaner solution)
2023-04-07T15:47:07Z
https://github.com/huggingface/datasets/pull/5704#issuecomment-1500403720
https://api.github.com/repos/huggingface/datasets/issues/5704
IC_kwDODunzps5ZblgI
https://api.github.com/repos/huggingface/datasets/issues/comments/1500403720/reactions
2023-04-07T15:47:07Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1500403720
https://avatars.githubusercontent.com/u/47462742?v=4
https://api.github.com/users/mariosasko/events{/privacy}
https://api.github.com/users/mariosasko/followers
https://api.github.com/users/mariosasko/following{/other_user}
https://api.github.com/users/mariosasko/gists{/gist_id}
https://github.com/mariosasko
mariosasko
MDQ6VXNlcjQ3NDYyNzQy
https://api.github.com/users/mariosasko/orgs
https://api.github.com/users/mariosasko/received_events
https://api.github.com/users/mariosasko/repos
https://api.github.com/users/mariosasko/starred{/owner}{/repo}
https://api.github.com/users/mariosasko/subscriptions
User
https://api.github.com/users/mariosasko
public
NONE
Ooof. Sorry, I should have checked that more thoroughly then! I would say we could just add that check and only use my approach if the root is "", which would still be faster in many cases, but it sounds like you have a better solution on the way. Thanks for the feedback Mario.
2023-04-07T16:10:54Z
https://github.com/huggingface/datasets/pull/5704#issuecomment-1500425499
https://api.github.com/repos/huggingface/datasets/issues/5704
IC_kwDODunzps5Zbq0b
https://api.github.com/repos/huggingface/datasets/issues/comments/1500425499/reactions
2023-04-07T16:10:54Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1500425499
https://avatars.githubusercontent.com/u/35013374?v=4
https://api.github.com/users/semajyllek/events{/privacy}
https://api.github.com/users/semajyllek/followers
https://api.github.com/users/semajyllek/following{/other_user}
https://api.github.com/users/semajyllek/gists{/gist_id}
https://github.com/semajyllek
semajyllek
MDQ6VXNlcjM1MDEzMzc0
https://api.github.com/users/semajyllek/orgs
https://api.github.com/users/semajyllek/received_events
https://api.github.com/users/semajyllek/repos
https://api.github.com/users/semajyllek/starred{/owner}{/repo}
https://api.github.com/users/semajyllek/subscriptions
User
https://api.github.com/users/semajyllek
public
I reimplemented fsspec.spec.glob() in `hffilesystem.py` as `_glob`,  used it in `_resolve_single_pattern_in_dataset_repository` only, and saw a 20% speedup in times to load the config, on average. 

That's not much when usually this step takes only 2-3 seconds for most datasets, but in this particular case,  `bigcode/the-stack-dedup` , the loading time to get the config (not download the entire 6tb dataset, of course), went from ~170 secs to ~20 secs. 

What makes this work is this code in `_glob`:

```  
      if self.dir_cache is not None:
            allpaths = self.dir_cache
        else:
            allpaths = self.find(root, maxdepth=depth, withdirs=True, detail=True, **kwargs)
```

I also had to `import glob.has_magic( )` for `_glob()` (confusing, I know).

I hope there is no issue with copying most of the code from `fsspec.spec.glob`, as it is a BSD 3-Clause License, 
and I left a comment about this in the docstring of` _glob()`, that we may want to delete.

As mentioned, I evaluated the speedup across a random selection of about 1000 datasets (not all 27k+), and verified that old_config.eq(new_method_config) with the build in method, but deleted this test and related code changes on the subsequent commit. It's in the commit history if anyone wants to see it. (Note this does not include the outlier of  `bigcode/the-stack-dedup`

|   |   old_time     |    new _time |     diff   |  pct_diff |
| -- | -- | -- | -- | -- |
| mean  |    3.340 |    2.642 |    0.698 |   18.404 |
| min   |    2.024   |  1.976   |  -0.840  | -37.634 |
| max   |  66.582  |   41.517 |   30.927  |  85.538 |