Repository: huggingface/datasets
Pull request: Fix script fetching and local path handling in `inspect_dataset` and `inspect_metric`
Fix #4348

Problem statement:
`inspect` functions can't fetch dataset script from the Hub
The `inspect_dataset` and `inspect_metric` functions are unable to retrieve a dataset or metric script from the Hub and store it locally at the specified `local_path`:

```py
>>> from datasets import inspect_dataset
>>> inspect_dataset('rotten_tomatoes', local_path='path/to/my/local/folder')

FileNotFoundError: Couldn't find a dataset script at /content/rotten_tomatoes/rotten_tomatoes.py or any data file in the same directory.
```

Hints from issue discussion:
Hi, thanks for reporting! `git bisect` points to #2986 as the PR that introduced the bug. Since then, there have been some additional changes to the loading logic, and in the current state, `force_local_path` (set via `local_path`) forbids pulling a script from the internet instead of downloading it: https://github.com/huggingface/datasets/blob/cfae0545b2ba05452e16136cacc7d370b4b186a1/src/datasets/inspect.py#L89-L91

cc @lhoestq: `force_local_path` is only used in `inspect_dataset` and `inspect_metric`. Is it OK if we revert the behavior to match the old one?
Good catch ! Yea I think it's fine :)

Commit message: Iterate on copy when deleting list items

Patch to review:
diff --git a/src/datasets/download/download_manager.py b/src/datasets/download/download_manager.py
index 3783fc2c178..8ba7434180a 100644
--- a/src/datasets/download/download_manager.py
+++ b/src/datasets/download/download_manager.py
@@ -128,7 +128,7 @@ def _iter_from_paths(cls, urlpaths: Union[str, List[str]]) -> Generator[str, Non
                 yield urlpath
             else:
                 for dirpath, dirnames, filenames in os.walk(urlpath):
-                    for i, dirname in enumerate(dirnames):
+                    for i, dirname in enumerate(dirnames[:]):
                         if dirname.startswith((".", "__")):
                             # skipping hidden directories; prune the search
                             del dirnames[i]
diff --git a/src/datasets/download/streaming_download_manager.py b/src/datasets/download/streaming_download_manager.py
index cd1fa552fe0..9b80677594a 100644
--- a/src/datasets/download/streaming_download_manager.py
+++ b/src/datasets/download/streaming_download_manager.py
@@ -759,7 +759,7 @@ def _iter_from_urlpaths(
                 yield urlpath
             else:
                 for dirpath, dirnames, filenames in xwalk(urlpath, use_auth_token=use_auth_token):
-                    for i, dirname in enumerate(dirnames):
+                    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)
diff --git a/src/datasets/inspect.py b/src/datasets/inspect.py
index dbfbca63b35..a3d7d564fe0 100644
--- a/src/datasets/inspect.py
+++ b/src/datasets/inspect.py
@@ -15,6 +15,10 @@
 # Lint as: python3
 """ List and inspect datasets and metrics."""
 
+import inspect
+import os
+import shutil
+from pathlib import PurePath
 from typing import Dict, List, Mapping, Optional, Sequence, Union
 
 import huggingface_hub
@@ -30,6 +34,7 @@
     load_dataset_builder,
     metric_module_factory,
 )
+from .utils.file_utils import relative_to_absolute_path
 from .utils.logging import get_logger
 from .utils.version import Version
 
@@ -118,13 +123,24 @@ def inspect_dataset(path: str, local_path: str, download_config: Optional[Downlo
         **download_kwargs (additional keyword arguments): Optional arguments for [`DownloadConfig`] which will override
             the attributes of `download_config` if supplied.
     """
-    dataset_module = dataset_module_factory(
-        path, download_config=download_config, force_local_path=local_path, **download_kwargs
-    )
+    dataset_module = dataset_module_factory(path, download_config=download_config, **download_kwargs)
+    builder_cls = import_main_class(dataset_module.module_path, dataset=True)
+    module_source_path = inspect.getsourcefile(builder_cls)
+    module_source_dirpath = os.path.dirname(module_source_path)
+    for dirpath, dirnames, filenames in os.walk(module_source_dirpath):
+        dst_dirpath = os.path.join(local_path, os.path.relpath(dirpath, module_source_dirpath))
+        os.makedirs(dst_dirpath, exist_ok=True)
+        for i, dirname in enumerate(dirnames[:]):
+            if dirname == "__pycache__":
+                del dirnames[i]
+            for filename in filenames:
+                shutil.copy2(os.path.join(dirpath, filename), os.path.join(dst_dirpath, filename))
+        shutil.copystat(dirpath, dst_dirpath)
+    local_path = relative_to_absolute_path(local_path)
     print(
         f"The processing script for dataset {path} can be inspected at {local_path}. "
-        f"The main class is in {dataset_module.module_path}. "
-        f"You can modify this processing script and use it with `datasets.load_dataset({local_path})`."
+        f"The main class is in {module_source_dirpath}. "
+        f'You can modify this processing script and use it with `datasets.load_dataset("{PurePath(local_path).as_posix()}")`.'
     )
 
 
@@ -143,13 +159,24 @@ def inspect_metric(path: str, local_path: str, download_config: Optional[Downloa
         download_config (Optional ``datasets.DownloadConfig``): specific download configuration parameters.
         **download_kwargs (additional keyword arguments): optional attributes for DownloadConfig() which will override the attributes in download_config if supplied.
     """
-    metric_module = metric_module_factory(
-        path, download_config=download_config, force_local_path=local_path, **download_kwargs
-    )
+    metric_module = metric_module_factory(path, download_config=download_config, **download_kwargs)
+    builder_cls = import_main_class(metric_module.module_path, dataset=False)
+    module_source_path = inspect.getsourcefile(builder_cls)
+    module_source_dirpath = os.path.dirname(module_source_path)
+    for dirpath, dirnames, filenames in os.walk(module_source_dirpath):
+        dst_dirpath = os.path.join(local_path, os.path.relpath(dirpath, module_source_dirpath))
+        os.makedirs(dst_dirpath, exist_ok=True)
+        for i, dirname in enumerate(dirnames[:]):
+            if dirname == "__pycache__":
+                del dirnames[i]
+            for filename in filenames:
+                shutil.copy2(os.path.join(dirpath, filename), os.path.join(dst_dirpath, filename))
+        shutil.copystat(dirpath, dst_dirpath)
+    local_path = relative_to_absolute_path(local_path)
     print(
         f"The processing scripts for metric {path} can be inspected at {local_path}. "
-        f"The main class is in {metric_module.module_path}. "
-        f"You can modify this processing scripts and use it with `datasets.load_metric({local_path})`."
+        f"The main class is in {module_source_dirpath}. "
+        f'You can modify this processing scripts and use it with `datasets.load_metric("{PurePath(local_path).as_posix()}")`.'
     )
 
 
@@ -219,7 +246,6 @@ def get_dataset_config_names(
     revision: Optional[Union[str, Version]] = None,
     download_config: Optional[DownloadConfig] = None,
     download_mode: Optional[DownloadMode] = None,
-    force_local_path: Optional[str] = None,
     dynamic_modules_path: Optional[str] = None,
     data_files: Optional[Union[Dict, List, str]] = None,
     **download_kwargs,
@@ -241,8 +267,6 @@ def get_dataset_config_names(
             Specifying a version that is different from your local version of the lib might cause compatibility issues.
         download_config (:class:`DownloadConfig`, optional): Specific download configuration parameters.
         download_mode (:class:`DownloadMode`, default ``REUSE_DATASET_IF_EXISTS``): Download/generate mode.
-        force_local_path (Optional str): Optional path to a local path to download and prepare the script to.
-            Used to inspect or modify the script folder.
         dynamic_modules_path (Optional str, defaults to HF_MODULES_CACHE / "datasets_modules", i.e. ~/.cache/huggingface/modules/datasets_modules):
             Optional path to the directory in which the dynamic modules are saved. It must have been initialized with :obj:`init_dynamic_modules`.
             By default the datasets and metrics are stored inside the `datasets_modules` module.
@@ -274,7 +298,6 @@ def get_dataset_config_names(
         revision=revision,
         download_config=download_config,
         download_mode=download_mode,
-        force_local_path=force_local_path,
         dynamic_modules_path=dynamic_modules_path,
         data_files=data_files,
         **download_kwargs,
diff --git a/src/datasets/load.py b/src/datasets/load.py
index 98bdeddedb5..23f6e1ce92a 100644
--- a/src/datasets/load.py
+++ b/src/datasets/load.py
@@ -1100,7 +1100,6 @@ def dataset_module_factory(
     revision: Optional[Union[str, Version]] = None,
     download_config: Optional[DownloadConfig] = None,
     download_mode: Optional[DownloadMode] = None,
-    force_local_path: Optional[str] = None,
     dynamic_modules_path: Optional[str] = None,
     data_dir: Optional[str] = None,
     data_files: Optional[Union[Dict, List, str, DataFilesDict]] = None,
@@ -1142,8 +1141,6 @@ def dataset_module_factory(
               You can specify a different version that the default "main" by using a commit sha or a git tag of the dataset repository.
         download_config (:class:`DownloadConfig`, optional): Specific download configuration parameters.
         download_mode (:class:`DownloadMode`, default ``REUSE_DATASET_IF_EXISTS``): Download/generate mode.
-        force_local_path (Optional str): Optional path to a local path to download and prepare the script to.
-            Used to inspect or modify the script folder.
         dynamic_modules_path (Optional str, defaults to HF_MODULES_CACHE / "datasets_modules", i.e. ~/.cache/huggingface/modules/datasets_modules):
             Optional path to the directory in which the dynamic modules are saved. It must have been initialized with :obj:`init_dynamic_modules`.
             By default the datasets and metrics are stored inside the `datasets_modules` module.
@@ -1212,7 +1209,7 @@ def dataset_module_factory(
             path, data_dir=data_dir, data_files=data_files, download_mode=download_mode
         ).get_module()
     # Try remotely
-    elif is_relative_path(path) and path.count("/") <= 1 and not force_local_path:
+    elif is_relative_path(path) and path.count("/") <= 1:
         try:
             _raise_if_offline_mode_is_enabled()
             if path.count("/") == 0:  # even though the dataset is on the Hub, we get it from GitHub for now
@@ -1292,7 +1289,6 @@ def metric_module_factory(
     revision: Optional[Union[str, Version]] = None,
     download_config: Optional[DownloadConfig] = None,
     download_mode: Optional[DownloadMode] = None,
-    force_local_path: Optional[str] = None,
     dynamic_modules_path: Optional[str] = None,
     **download_kwargs,
 ) -> MetricModule:
@@ -1320,8 +1316,6 @@ def metric_module_factory(
             Specifying a version that is different from your local version of the lib might cause compatibility issues.
         download_config (:class:`DownloadConfig`, optional): Specific download configuration parameters.
         download_mode (:class:`DownloadMode`, default ``REUSE_DATASET_IF_EXISTS``): Download/generate mode.
-        force_local_path (Optional str): Optional path to a local path to download and prepare the script to.
-            Used to inspect or modify the script folder.
         dynamic_modules_path (Optional str, defaults to HF_MODULES_CACHE / "datasets_modules", i.e. ~/.cache/huggingface/modules/datasets_modules):
             Optional path to the directory in which the dynamic modules are saved. It must have been initialized with :obj:`init_dynamic_modules`.
             By default, the datasets and metrics are stored inside the `datasets_modules` module.
@@ -1353,7 +1347,7 @@ def metric_module_factory(
         return LocalMetricModuleFactory(
             combined_path, download_mode=download_mode, dynamic_modules_path=dynamic_modules_path
         ).get_module()
-    elif is_relative_path(path) and path.count("/") == 0 and not force_local_path:
+    elif is_relative_path(path) and path.count("/") == 0:
         try:
             return GithubMetricModuleFactory(
                 path,
diff --git a/tests/test_inspect.py b/tests/test_inspect.py
index 8d46f0183a4..5774ea8b889 100644
--- a/tests/test_inspect.py
+++ b/tests/test_inspect.py
@@ -1,6 +1,31 @@
+import os
+
 import pytest
 
-from datasets import get_dataset_config_info, get_dataset_config_names, get_dataset_infos, get_dataset_split_names
+from datasets import (
+    get_dataset_config_info,
+    get_dataset_config_names,
+    get_dataset_infos,
+    get_dataset_split_names,
+    inspect_dataset,
+    inspect_metric,
+)
+
+
+@pytest.mark.parametrize("path", ["paws", "csv"])
+def test_inspect_dataset(path, tmp_path):
+    inspect_dataset(path, tmp_path)
+    script_name = path + ".py"
+    assert script_name in os.listdir(tmp_path)
+    assert "__pycache__" not in os.listdir(tmp_path)
+
+
+@pytest.mark.parametrize("path", ["accuracy"])
+def test_inspect_metric(path, tmp_path):
+    inspect_metric(path, tmp_path)
+    script_name = path + ".py"
+    assert script_name in os.listdir(tmp_path)
+    assert "__pycache__" not in os.listdir(tmp_path)
 
 
 @pytest.mark.parametrize(


Code review comments:
diff_hunk: @@ -118,13 +123,24 @@ def inspect_dataset(path: str, local_path: str, download_config: Optional[Downlo
         **download_kwargs (additional keyword arguments): Optional arguments for [`DownloadConfig`] which will override
             the attributes of `download_config` if supplied.
     """
-    dataset_module = dataset_module_factory(
-        path, download_config=download_config, force_local_path=local_path, **download_kwargs
-    )
+    dataset_module = dataset_module_factory(path, download_config=download_config, **download_kwargs)
+    builder_cls = import_main_class(dataset_module.module_path, dataset=True)
+    module_source_path = inspect.getsourcefile(builder_cls)
+    module_source_dirpath = os.path.dirname(module_source_path)
+    for dirpath, dirnames, filenames in os.walk(module_source_dirpath):
+        dst_dirpath = os.path.join(local_path, os.path.relpath(dirpath, module_source_dirpath))
+        os.makedirs(dst_dirpath, exist_ok=True)
+        for i, dirname in enumerate(dirnames[:]):
+            if dirname == "__pycache__":
+                del dirnames[i]
path: src/datasets/inspect.py
text: @user1:
Same here, and I guess you can ignore directories that start with "." and "__" to be more general, as we do in the download manager
```suggestion
        for i, dirname in enumerate(dirnames):
            if dirname.startswith((".", "__")):
                continue
```

diff_hunk: @@ -759,7 +759,7 @@ def _iter_from_urlpaths(
                 yield urlpath
             else:
                 for dirpath, dirnames, filenames in xwalk(urlpath, use_auth_token=use_auth_token):
-                    for i, dirname in enumerate(dirnames):
+                    for i, dirname in enumerate(dirnames[:]):
path: src/datasets/download/streaming_download_manager.py
text: @user1:
Why is this needed ? Is it related to the `del dirnames[i]` call ? If so maybe it's simpler to just remove `del dirnames[i]` instead. I feel like this optimization adds more confusion that it brings value

Otherwise if you think we should keep it, feel free to add a comment to explain why

@author:
Yes. There is a comment that explains this inside the loop... It's outside of it now. And I removed the `[:]` syntax.

diff_hunk: @@ -143,13 +159,24 @@ def inspect_metric(path: str, local_path: str, download_config: Optional[Downloa
         download_config (Optional ``datasets.DownloadConfig``): specific download configuration parameters.
         **download_kwargs (additional keyword arguments): optional attributes for DownloadConfig() which will override the attributes in download_config if supplied.
     """
-    metric_module = metric_module_factory(
-        path, download_config=download_config, force_local_path=local_path, **download_kwargs
-    )
+    metric_module = metric_module_factory(path, download_config=download_config, **download_kwargs)
+    builder_cls = import_main_class(metric_module.module_path, dataset=False)
+    module_source_path = inspect.getsourcefile(builder_cls)
+    module_source_dirpath = os.path.dirname(module_source_path)
+    for dirpath, dirnames, filenames in os.walk(module_source_dirpath):
+        dst_dirpath = os.path.join(local_path, os.path.relpath(dirpath, module_source_dirpath))
+        os.makedirs(dst_dirpath, exist_ok=True)
+        for i, dirname in enumerate(dirnames[:]):
+            if dirname == "__pycache__":
+                del dirnames[i]
path: src/datasets/inspect.py
text: @user1:
Same here
```suggestion
        for i, dirname in enumerate(dirnames):
            if dirname.startswith((".", "__")):
                continue
```