Better double asterisks `**` support #1329

john-jam · 2023-08-06T11:25:34Z

Description

This PR:

makes the glob method compliant with posix rules and add better support for double asterisks ** in globs.
supports maxdepth for the glob method.
fixes the copy, get and put methods to not create unwanted directories on some scenarios.

I added some tests to check the posix rules against the python glob.glob implementation and the bash globstar support. Both do not support double asterisks with other prefix/suffix than / so for example **.json is the same as *.json and not the same as **/*.json. But since this repository already supported that well, this feature is kept and patterns like ab**cd are still working properly (returning paths like ab/ef/cd).

New rules:

the root dir is returned when using find. It is because of the posix ** rule compliance. For example, it should return an empty string among the other paths when using ** or return ab among the other paths when using ab/**.
when using a trailing slash in globs, it returns only directories.
the double asterisks ** matches 0+ directories.
the maxdepth option is applied on the first ** when using a glob.

References

john-jam · 2023-08-08T08:37:01Z

@martindurant @ianthomas23 I can't run all the tests on my machine locally (errors with dask and ftp). Could you trigger a CI run?

martindurant · 2023-08-08T13:56:36Z

You are failing on the range of things that we typically see break whenever you change anything about how paths work :|. In particular, you will find it difficult to reconcile what happens on windows without having a system of your own to run tests on.

martindurant · 2023-08-08T13:57:51Z

fsspec/implementations/local.py

@@ -65,10 +65,6 @@ def ls(self, path, detail=False, **kwargs):
        else:
            return [posixpath.join(path, f) for f in os.listdir(path)]

-    def glob(self, path, **kwargs):
-        path = self._strip_protocol(path)


Removing _strip_protocol from the flow here is likely the cause of all the windows errors, because for LocalFileSystem, it also converts windows-style paths to posix.

This is what caused this issue: #1322
How about using make_posix_path helper here then?

I fixed the windows tests by using make_path_posix on the expected outputs in the tests when required: https://github.com/john-jam/filesystem_spec/actions/runs/5807521471/job/15742517288

martindurant · 2023-08-08T14:06:20Z

@ianthomas23 , do you think you will have time to go through this?

ianthomas23 · 2023-08-08T14:33:13Z

@ianthomas23 , do you think you will have time to go through this?

Yes I will, but later in the week.

john-jam · 2023-08-08T16:01:33Z

You are failing on the range of things that we typically see break whenever you change anything about how paths work

Yes, there's a lot of updates to force glob compliant with posix rules but maybe some of them are not wanted? For example the following posix rules could be reverted if you think it's not worth it:

the root dir is returned when using double asterisks **.
when using a trailing slash in globs, it returns only directories.

.github/workflows/main.yaml

This reverts commit 52b0e65.

ianthomas23

This is good to merge now as far as I am concerned. I'll give @martindurant the final say.

When this is merged we can re-run the CI of the s3fs and gcsfs PRs, and I will review them properly. They look fine at initial glance. Ideally we would want to merge both of those quite quickly after this so that everything is consistent before the next releases.

I've closed my gcsfs abstract tests PR fsspec/gcsfs#535 as fsspec/gcsfs#572 is definitely a better solution.

john-jam · 2023-08-21T02:22:40Z

@ianthomas23 I changed the posix tests with python glob and bash glob to use relative paths instead of absolute paths.
This allowed me to detect an incoherence with the ** use case. fsspec was returning an empty string "" for the current directory but the python glob and bash glob, both don't return an empty string "" for the current dir when using relative paths.

When using a prefix though (e.g. dir/**), the three implementations return the root folder (e.g. dir).

I fixed it in this commit.

@martindurant Please let me know if there's any work still to be done in this PR for it to be merged soon?

john-jam · 2023-08-22T06:33:14Z

@ianthomas23 @martindurant I found another edge case with gcsfs. I created two PRs (here and here) but need to merge this PR before. I'll change the PR targets accordingly once this one is merged.

martindurant · 2023-08-22T13:47:30Z

Merging now, and then we can deal with the small leftovers.

john-jam · 2023-08-28T01:00:13Z

@martindurant @ianthomas23 Thanks for the merge of this PR and the ones in s3fs and gcsfs!

Do you know approx. when will you release a new version of those 3 repos? And also if you plan to include those changes in gcsfs?

martindurant · 2023-08-30T15:00:37Z

Releases should be out this week, so long as lingering PRs are cleaned up.

`fsspec` upgrade in fsspec/filesystem_spec#1329 makes 1. when using a trailing slash in globs, it returns only directories. 2. the double asterisks ** matches 0+ directories. This PR fix the tests accordingly.

…tory (#37558) ### Rationale for this change There has been some changes in the way fsspec lists the directories with new version 2023.9.0, see fsspec/filesystem_spec#1329, which caused our tests to start failing. ### What changes are included in this PR? This PR updates the `get_file_info_selector` in [FSSpecHandler](https://arrow.apache.org/docs/_modules/pyarrow/fs.html#FSSpecHandler) class to keep the behaviour of our spec. ### Are there any user-facing changes? No. * Closes: #37555 Authored-by: AlenkaF <[email protected]> Signed-off-by: AlenkaF <[email protected]>

… directory (apache#37558) ### Rationale for this change There has been some changes in the way fsspec lists the directories with new version 2023.9.0, see fsspec/filesystem_spec#1329, which caused our tests to start failing. ### What changes are included in this PR? This PR updates the `get_file_info_selector` in [FSSpecHandler](https://arrow.apache.org/docs/_modules/pyarrow/fs.html#FSSpecHandler) class to keep the behaviour of our spec. ### Are there any user-facing changes? No. * Closes: apache#37555 Authored-by: AlenkaF <[email protected]> Signed-off-by: AlenkaF <[email protected]>

john-jam added 3 commits August 6, 2023 20:20

chore: add posix compliance tests

a85d689

feat: make glob posix compliant

23ece4f

fix: remove unnecessary trailing slash removal

63aeb0f

john-jam mentioned this pull request Aug 6, 2023

Avoid destination directory creation when using glob double asterisk(**) #1321

Closed

john-jam added 8 commits August 7, 2023 14:13

Support the **/* case

0883de9

feat: support maxdepth for globs

d42d09a

chore: refactor tests and add maxdepth tests

bd41c2f

fix: pass the maxdepth option to glob in expand_path

1c1aa0d

fix: remove unused is_dir option in other_paths

1abb2c6

fix: avoid copying unwanted directories

1b2463c

feat: refactor copy, get and put tests

31424bd

feat: add missing tests

69bb15f

john-jam marked this pull request as ready for review August 8, 2023 08:15

john-jam added 3 commits August 8, 2023 17:42

fix: fix other tests to include root dirs

2dc4338

fix: fix other repos import

45f828d

fix: fix missing tests with root dir

2f33e1b

fix: add missing async code changes

6362058

martindurant reviewed Aug 8, 2023

View reviewed changes

tmp: use personal repos for friends

52b0e65

john-jam added 2 commits August 8, 2023 23:47

fix: update the wront exist test case

b20ce62

fix: update http glob implementation

8680aef

john-jam force-pushed the better-double-asterisk-support branch from e332f05 to 8680aef Compare August 8, 2023 15:12

fix: handle file not found when removing glob target test

ee514b6

john-jam commented Aug 8, 2023

View reviewed changes

.github/workflows/main.yaml Outdated Show resolved Hide resolved

fix: test empty ls results only against fs that supports empty dir

37aabad

Revert "tmp: use personal repos for friends"

2a089d3

This reverts commit 52b0e65.

john-jam requested review from ianthomas23 and martindurant August 16, 2023 01:51

This was referenced Aug 16, 2023

Better double asterisk support fsspec/gcsfs#572

Merged

Better double asterisk support fsspec/s3fs#769

Merged

ianthomas23 approved these changes Aug 17, 2023

View reviewed changes

fix: use relative paths for posix tests

85b7ae0

fix: remove comment

495d986

This was referenced Aug 22, 2023

Test to copy directory with files with same name prefix john-jam/filesystem_spec#1

Closed

Test to copy directory with files with same name prefix john-jam/gcsfs#1

Closed

martindurant merged commit c3b4bc3 into fsspec:master Aug 22, 2023
9 of 11 checks passed

john-jam deleted the better-double-asterisk-support branch August 28, 2023 00:57

Wauplin mentioned this pull request Sep 4, 2023

Fix fsspec tests in ci huggingface/huggingface_hub#1635

Merged

This was referenced Sep 5, 2023

[CI][Python] test_get_file_info_with_selector is failed on AppVeyor apache/arrow#37555

Closed

GH-37555: [Python] Update get_file_info_selector to ignore base directory apache/arrow#37558

Merged

albertvillanova mentioned this pull request Sep 5, 2023

Unpin fsspec < 2023.9.0 huggingface/datasets#6214

Closed

john-jam mentioned this pull request Sep 5, 2023

glob result for s3 folder different in 2023.6.0 and 2023.9.0 versions fsspec/s3fs#783

Closed

ap-- mentioned this pull request Sep 7, 2023

Fix glob behaviour fsspec/adlfs#422

Merged

seanses mentioned this pull request Sep 8, 2023

fix fsspec test due to fsspec upgrade xetdata/pyxet#93

Merged

mariosasko mentioned this pull request Sep 19, 2023

Pin fsspec to use default expand_path huggingface/huggingface_hub#1681

Merged

danielhfrank mentioned this pull request Mar 13, 2024

Update CI and switch to python 3.11 related-sciences/articat#57

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better double asterisks `**` support #1329

Better double asterisks `**` support #1329

john-jam commented Aug 6, 2023 •

edited

Loading

john-jam commented Aug 8, 2023

martindurant commented Aug 8, 2023

martindurant Aug 8, 2023

john-jam Aug 8, 2023

john-jam Aug 9, 2023 •

edited

Loading

martindurant commented Aug 8, 2023

ianthomas23 commented Aug 8, 2023

john-jam commented Aug 8, 2023

ianthomas23 left a comment

john-jam commented Aug 21, 2023

john-jam commented Aug 22, 2023

martindurant commented Aug 22, 2023

john-jam commented Aug 28, 2023

martindurant commented Aug 30, 2023

Better double asterisks ** support #1329

Better double asterisks ** support #1329

Conversation

john-jam commented Aug 6, 2023 • edited Loading

Description

References

john-jam commented Aug 8, 2023

martindurant commented Aug 8, 2023

martindurant Aug 8, 2023

Choose a reason for hiding this comment

john-jam Aug 8, 2023

Choose a reason for hiding this comment

john-jam Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

martindurant commented Aug 8, 2023

ianthomas23 commented Aug 8, 2023

john-jam commented Aug 8, 2023

ianthomas23 left a comment

Choose a reason for hiding this comment

john-jam commented Aug 21, 2023

john-jam commented Aug 22, 2023

martindurant commented Aug 22, 2023

john-jam commented Aug 28, 2023

martindurant commented Aug 30, 2023

Better double asterisks `**` support #1329

Better double asterisks `**` support #1329

john-jam commented Aug 6, 2023 •

edited

Loading

john-jam Aug 9, 2023 •

edited

Loading