Fix regex get_data_files formatting for base paths
#6322
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ZachNagengast
changed the title
get_data_files formatting for url base pathsget_data_files formatting for base paths
albertvillanova
requested changes
Oct 20, 2023
Member
There was a problem hiding this comment.
Thanks for the proposed fix, @ZachNagengast.
EDIT:
The reason why I used the the glob_pattern_to_regex in the entire pattern is because otherwise I got an error for Windows local paths: a base_path like 'C:\\Users\\runneradmin... made the function string_to_dict raise re.error: incomplete escape \U at position 2
See: https://github.com/huggingface/datasets/actions/runs/6544904352/job/17772361643
We should include a test that includes the case you mention, and find a solution that works for all cases.
That issue was fixed once we pass the base_path as POSIX.
Maybe we could add a test that fails in the case you mention.
|
The documentation is not available anymore as the PR was closed or merged. |
Contributor
Author
What is the expected inputs and outputs for the windows
I'm not sure what you meant by that, are there still changes needed? |
Member
|
We took the liberty of continuing this PR to include it in today's patch release :) |
| splits: Set[str] = { | ||
| string_to_dict(xbasename(p), glob_pattern_to_regex(xbasename(split_pattern)))["split"] | ||
| for p in data_files | ||
| } |
Member
There was a problem hiding this comment.
If you are matching just in the basename, then what is the point of having 2 kinds of patterns?
- ALL_SPLIT_PATTERNS:
data/{split}-[0-9][0-9][0-9][0-9][0-9]-of-[0-9][0-9][0-9][0-9][0-9]*.* - ALL_DEFAULT_PATTERNS:
**/*[{sep}/]{keyword}[{sep}/]**
Maybe I'm missing something, but why do we need the former? I would naively say the latter contains the former.
Member
There was a problem hiding this comment.
Only ALL_SPLIT_PATTERNS are parsed to infer custom split names.
While the second only detects train/valid/test
Member
There was a problem hiding this comment.
OK, and what is the point of the directory data/ in ALL_SPLIT_PATTERNS if we only match the basename?
Member
There was a problem hiding this comment.
This is for old push_to_hub to work: they push custom splits using this pattern in the data directory.
New push_to_hub have some YAML to specify the pattern to use, so get_data_patterns isn't called
Member
There was a problem hiding this comment.
OK, all clear now. Thanks.
albertvillanova
approved these changes
Oct 23, 2023
lhoestq
added a commit
that referenced
this pull request
Oct 23, 2023
* Fix regex from formatting url base_path * Test test_get_data_patterns from Hub * simply match basename instead * more tests * minor * remove comment --------- Co-authored-by: Albert Villanova del Moral <[email protected]> Co-authored-by: Quentin Lhoest <[email protected]> Co-authored-by: Quentin Lhoest <[email protected]>
Show benchmarksPyArrow==8.0.0 Show updated benchmarks!Benchmark: benchmark_array_xd.json
Benchmark: benchmark_getitem_100B.json
Benchmark: benchmark_indices_mapping.json
Benchmark: benchmark_iterating.json
Benchmark: benchmark_map_filter.json
Show updated benchmarks!Benchmark: benchmark_array_xd.json
Benchmark: benchmark_getitem_100B.json
Benchmark: benchmark_indices_mapping.json
Benchmark: benchmark_iterating.json
Benchmark: benchmark_map_filter.json
|
albertvillanova
added a commit
that referenced
this pull request
Oct 24, 2023
* Fix regex from formatting url base_path * Test test_get_data_patterns from Hub * simply match basename instead * more tests * minor * remove comment --------- Co-authored-by: Albert Villanova del Moral <[email protected]> Co-authored-by: Quentin Lhoest <[email protected]> Co-authored-by: Quentin Lhoest <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this pr #6309, it is formatting the entire base path into regex, which results in the undesired formatting error
doesn't match the patternbecause of the line inglob_pattern_to_regex:.replace("//", "/"):hf://datasets/...hf:/datasets/...This fix will only convert the
split_patternto regex and keep thebase_pathunchanged.cc @albertvillanova hopefully this still works with your implementation