Repository: huggingface/datasets
Pull request: Skip hidden files/directories in data files resolution and `iter_files`
Fix #4115 

Problem statement:
ImageFolder add option to ignore some folders like '.ipynb_checkpoints'
**Is your feature request related to a problem? Please describe.**
I sometimes like to peek at the dataset images from jupyterlab. thus '.ipynb_checkpoints' folder appears where my dataset is and (just realized) leads to accidental duplicate image additions. I think this is an easy enough thing to miss especially if the dataset is very large.

**Describe the solution you'd like**
maybe have an option `ignore` or something .gitignore style
`dataset = load_dataset("imagefolder", data_dir="./data/original", ignore="regex?")`

**Describe alternatives you've considered**
Could filter out manually

Hints from issue discussion:
Maybe it would be nice to ignore private dirs like this one (ones starting with `.`) by default. 

CC @mariosasko 
Maybe we can add a `ignore_hidden_files` flag to the builder configs of our packaged loaders (to be consistent across all of them), wdyt @lhoestq @albertvillanova? 
I think they should always ignore them actually ! Not sure if adding a flag would be helpful
@lhoestq But what if the user explicitly requests those files via regex?

`glob.glob` ignores hidden files (files starting with ".") by default unless they are explicitly requested, but fsspec's `glob` doesn't follow this behavior, which is probably a bug, so maybe we can raise an issue or open a PR in their repo?
> @lhoestq But what if the user explicitly requests those files via regex?

Usually hidden files are meant to be ignored. If they are data files, they must be placed outside a hidden directory in the first place right ? I think it's more sensible to explain this than adding a flag.

> glob.glob ignores hidden files (files starting with ".") by default unless they are explicitly requested, but fsspec's glob doesn't follow this behavior, which is probably a bug, so maybe we can raise an issue or open a PR in their repo?

After globbing using `fsspec`, we already ignore files that start with a `.` in `_resolve_single_pattern_locally` and `_resolve_single_pattern_in_dataset_repository`, I guess we can just account for parent directories as well ?

We could open an issue on `fsspec` but I think they won't change this since it's an important breaking change for them.

Commit message: Remove minor optimization

Patch to review:
diff --git a/src/datasets/data_files.py b/src/datasets/data_files.py
index dcbe2458963..bda19ddad69 100644
--- a/src/datasets/data_files.py
+++ b/src/datasets/data_files.py
@@ -122,7 +122,7 @@ def _resolve_single_pattern_locally(
     matched_paths = [
         Path(filepath).resolve()
         for filepath in glob_iter
-        if filepath.name not in data_files_ignore and not filepath.name.startswith(".")
+        if filepath.name not in data_files_ignore and not any(part.startswith((".", "__")) for part in filepath.parts)
     ]
     if allowed_extensions is not None:
         out = [
@@ -307,7 +307,7 @@ def _resolve_single_pattern_in_dataset_repository(
     matched_paths = [
         filepath
         for filepath in glob_iter
-        if filepath.name not in data_files_ignore and not filepath.name.startswith(".")
+        if filepath.name not in data_files_ignore and not any(part.startswith((".", "__")) for part in filepath.parts)
     ]
     if allowed_extensions is not None:
         out = [
diff --git a/src/datasets/download/download_manager.py b/src/datasets/download/download_manager.py
index c9de65b8cc3..3783fc2c178 100644
--- a/src/datasets/download/download_manager.py
+++ b/src/datasets/download/download_manager.py
@@ -122,10 +122,23 @@ def _iter_from_paths(cls, urlpaths: Union[str, List[str]]) -> Generator[str, Non
             urlpaths = [urlpaths]
         for urlpath in urlpaths:
             if os.path.isfile(urlpath):
+                if os.path.basename(urlpath).startswith((".", "__")):
+                    # skipping hidden files
+                    return
                 yield urlpath
             else:
-                for dirpath, _, filenames in os.walk(urlpath):
+                for dirpath, dirnames, filenames in os.walk(urlpath):
+                    for i, dirname in enumerate(dirnames):
+                        if dirname.startswith((".", "__")):
+                            # skipping hidden directories; prune the search
+                            del dirnames[i]
+                    if os.path.basename(dirpath).startswith((".", "__")):
+                        # skipping hidden directories
+                        continue
                     for filename in filenames:
+                        if filename.startswith((".", "__")):
+                            # skipping hidden files
+                            continue
                         yield os.path.join(dirpath, filename)
 
     @classmethod
diff --git a/src/datasets/download/mock_download_manager.py b/src/datasets/download/mock_download_manager.py
index 319d3d54111..4367a147a8b 100644
--- a/src/datasets/download/mock_download_manager.py
+++ b/src/datasets/download/mock_download_manager.py
@@ -211,7 +211,7 @@ def manage_extracted_files(self):
     def iter_archive(self, path):
         path = Path(path)
         for file_path in path.rglob("*"):
-            if file_path.is_file() and not file_path.name.startswith(".") and not file_path.name.startswith("__"):
+            if file_path.is_file() and not file_path.name.startswith((".", "__")):
                 yield file_path.relative_to(path).as_posix(), file_path.open("rb")
 
     def iter_files(self, paths):
@@ -219,8 +219,14 @@ def iter_files(self, paths):
             paths = [paths]
         for path in paths:
             if os.path.isfile(path):
+                if os.path.basename(path).startswith((".", "__")):
+                    return
                 yield path
             else:
                 for dirpath, _, filenames in os.walk(path):
+                    if os.path.basename(dirpath).startswith((".", "__")):
+                        continue
                     for filename in filenames:
+                        if filename.startswith((".", "__")):
+                            continue
                         yield os.path.join(dirpath, filename)
diff --git a/src/datasets/download/streaming_download_manager.py b/src/datasets/download/streaming_download_manager.py
index 793fe76a526..cd1fa552fe0 100644
--- a/src/datasets/download/streaming_download_manager.py
+++ b/src/datasets/download/streaming_download_manager.py
@@ -636,7 +636,7 @@ def xwalk(urlpath, use_auth_token: Optional[Union[str, bool]] = None):
     """
     main_hop, *rest_hops = str(urlpath).split("::")
     if is_local_path(main_hop):
-        return os.walk(main_hop)
+        yield from os.walk(main_hop)
     else:
         # walking inside a zip in a private repo requires authentication
         if rest_hops and (rest_hops[0].startswith("http://") or rest_hops[0].startswith("https://")):
@@ -718,7 +718,7 @@ def _iter_from_fileobj(cls, f) -> Generator[Tuple, None, None]:
                 continue
             if file_path is None:
                 continue
-            if os.path.basename(file_path).startswith(".") or os.path.basename(file_path).startswith("__"):
+            if os.path.basename(file_path).startswith((".", "__")):
                 # skipping hidden files
                 continue
             file_obj = stream.extractfile(tarinfo)
@@ -753,10 +753,24 @@ def _iter_from_urlpaths(
             urlpaths = [urlpaths]
         for urlpath in urlpaths:
             if xisfile(urlpath, use_auth_token=use_auth_token):
+                if xbasename(urlpath).startswith((".", "__")):
+                    # skipping hidden files
+                    return
                 yield urlpath
             else:
-                for dirpath, _, filenames in xwalk(urlpath, use_auth_token=use_auth_token):
+                for dirpath, dirnames, filenames in xwalk(urlpath, use_auth_token=use_auth_token):
+                    for i, dirname in enumerate(dirnames):
+                        if dirname.startswith((".", "__")):
+                            # skipping hidden directories; prune the search
+                            # (only works for local paths as fsspec's walk doesn't support the in-place modification)
+                            del dirnames[i]
+                    if xbasename(dirpath).startswith((".", "__")):
+                        # skipping hidden directories
+                        continue
                     for filename in filenames:
+                        if filename.startswith((".", "__")):
+                            # skipping hidden files
+                            continue
                         yield xjoin(dirpath, filename)
 
     @classmethod
diff --git a/src/datasets/packaged_modules/pandas/pandas.py b/src/datasets/packaged_modules/pandas/pandas.py
index 2e7c090fed3..3feba9cb264 100644
--- a/src/datasets/packaged_modules/pandas/pandas.py
+++ b/src/datasets/packaged_modules/pandas/pandas.py
@@ -17,12 +17,14 @@ def _split_generators(self, dl_manager):
             files = data_files
             if isinstance(files, str):
                 files = [files]
-            return [datasets.SplitGenerator(name=datasets.Split.TRAIN, gen_kwargs={"files": files})]
+            return [
+                datasets.SplitGenerator(name=datasets.Split.TRAIN, gen_kwargs={"files": dl_manager.iter_files(files)})
+            ]
         splits = []
         for split_name, files in data_files.items():
             if isinstance(files, str):
                 files = [files]
-            splits.append(datasets.SplitGenerator(name=split_name, gen_kwargs={"files": files}))
+            splits.append(datasets.SplitGenerator(name=split_name, gen_kwargs={"files": dl_manager.iter_files(files)}))
         return splits
 
     def _generate_tables(self, files):
diff --git a/src/datasets/packaged_modules/parquet/parquet.py b/src/datasets/packaged_modules/parquet/parquet.py
index 2f819f3a91f..1a3b5792852 100644
--- a/src/datasets/packaged_modules/parquet/parquet.py
+++ b/src/datasets/packaged_modules/parquet/parquet.py
@@ -34,12 +34,14 @@ def _split_generators(self, dl_manager):
             files = data_files
             if isinstance(files, str):
                 files = [files]
-            return [datasets.SplitGenerator(name=datasets.Split.TRAIN, gen_kwargs={"files": files})]
+            return [
+                datasets.SplitGenerator(name=datasets.Split.TRAIN, gen_kwargs={"files": dl_manager.iter_files(files)})
+            ]
         splits = []
         for split_name, files in data_files.items():
             if isinstance(files, str):
                 files = [files]
-            splits.append(datasets.SplitGenerator(name=split_name, gen_kwargs={"files": files}))
+            splits.append(datasets.SplitGenerator(name=split_name, gen_kwargs={"files": dl_manager.iter_files(files)}))
         return splits
 
     def _generate_tables(self, files):
diff --git a/tests/conftest.py b/tests/conftest.py
index 5aa997c1c57..f32ea203ca5 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -483,3 +483,26 @@ def zip_image_path(image_path, tmp_path_factory):
         f.write(image_path, arcname=os.path.basename(image_path))
         f.write(image_path, arcname=os.path.basename(image_path).replace(".jpg", "2.jpg"))
     return path
+
+
+@pytest.fixture(scope="session")
+def data_dir(tmp_path_factory):
+    data_dir = tmp_path_factory.mktemp("data_dir")
+
+    (data_dir / "subdir").mkdir()
+    with open(data_dir / "subdir" / "train.txt", "w") as f:
+        f.write("foo\n" * 10)
+    with open(data_dir / "subdir" / "test.txt", "w") as f:
+        f.write("bar\n" * 10)
+    # hidden file
+    with open(data_dir / "subdir" / ".test.txt", "w") as f:
+        f.write("bar\n" * 10)
+
+    # hidden directory
+    (data_dir / ".subdir").mkdir()
+    with open(data_dir / ".subdir" / "train.txt", "w") as f:
+        f.write("foo\n" * 10)
+    with open(data_dir / ".subdir" / "test.txt", "w") as f:
+        f.write("bar\n" * 10)
+
+    return data_dir
diff --git a/tests/test_data_files.py b/tests/test_data_files.py
index 394231a00b1..f8bb5d11305 100644
--- a/tests/test_data_files.py
+++ b/tests/test_data_files.py
@@ -21,6 +21,7 @@
 
 _TEST_PATTERNS = ["*", "**", "**/*", "*.txt", "data/*", "**/*.txt", "**/train.txt"]
 _FILES_TO_IGNORE = {".dummy", "README.md", "dummy_data.zip", "dataset_infos.json"}
+_DIRS_TO_IGNORE = {"data/.dummy_subdir"}
 _TEST_PATTERNS_SIZES = dict(
     [
         ("*", 0),
@@ -57,9 +58,24 @@ def complex_data_dir(tmp_path):
     with open(data_dir / "data" / "subdir" / "test.txt", "w") as f:
         f.write("bar\n" * 10)
 
+    (data_dir / "data" / ".dummy_subdir").mkdir()
+    with open(data_dir / "data" / ".dummy_subdir" / "train.txt", "w") as f:
+        f.write("foo\n" * 10)
+    with open(data_dir / "data" / ".dummy_subdir" / "test.txt", "w") as f:
+        f.write("bar\n" * 10)
+
     return str(data_dir)
 
 
+def is_relative_to(path, *other):
+    # A built-in method in Python 3.9+
+    try:
+        path.relative_to(*other)
+        return True
+    except ValueError:
+        return False
+
+
 @pytest.fixture
 def pattern_results(complex_data_dir):
     # We use fsspec glob as a reference for data files resolution from patterns.
@@ -78,7 +94,11 @@ def pattern_results(complex_data_dir):
         pattern: sorted(
             str(Path(path).resolve())
             for path in fsspec.filesystem("file").glob(os.path.join(complex_data_dir, pattern))
-            if Path(path).name not in _FILES_TO_IGNORE and Path(path).is_file()
+            if Path(path).name not in _FILES_TO_IGNORE
+            and not any(
+                is_relative_to(Path(path), os.path.join(complex_data_dir, dir_path)) for dir_path in _DIRS_TO_IGNORE
+            )
+            and Path(path).is_file()
         )
         for pattern in _TEST_PATTERNS
     }
diff --git a/tests/test_download_manager.py b/tests/test_download_manager.py
index 5d786c8e79c..63ac01b19ef 100644
--- a/tests/test_download_manager.py
+++ b/tests/test_download_manager.py
@@ -136,3 +136,10 @@ def test_iter_archive_file(tar_nested_jsonl_path):
             _test_jsonl(subpath, subfile)
     assert num_tar == 1
     assert num_jsonl == 2
+
+
+def test_iter_files(data_dir):
+    dl_manager = DownloadManager()
+    for num_file, file in enumerate(dl_manager.iter_files(data_dir), start=1):
+        pass
+    assert num_file == 2
diff --git a/tests/test_streaming_download_manager.py b/tests/test_streaming_download_manager.py
index b0bc09471b7..52be515689c 100644
--- a/tests/test_streaming_download_manager.py
+++ b/tests/test_streaming_download_manager.py
@@ -750,3 +750,10 @@ def test_iter_archive_file(tar_nested_jsonl_path):
             _test_jsonl(subpath, subfile)
     assert num_tar == 1
     assert num_jsonl == 2
+
+
+def test_iter_files(data_dir):
+    dl_manager = StreamingDownloadManager()
+    for num_file, file in enumerate(dl_manager.iter_files(data_dir), start=1):
+        pass
+    assert num_file == 2


Code review comments:
diff_hunk: @@ -483,3 +483,26 @@ def zip_image_path(image_path, tmp_path_factory):
         f.write(image_path, arcname=os.path.basename(image_path))
         f.write(image_path, arcname=os.path.basename(image_path).replace(".jpg", "2.jpg"))
     return path
+
+
+@pytest.fixture(scope="session")
+def data_dir(tmp_path_factory):
path: tests/conftest.py
text: @user1:
(nit) maybe better to just name it `data_dir_with_hidden_files` to make it more explicit