Repository: huggingface/huggingface_hub
Pull request #1692 (2023-09-23 15:52:16)

Address failing _check_disk_space() when path doesn't exist yet
Address issue #1690.

`_check_disk_space` fails when only provided with incomplete paths like `checkpoints/stabilityai/stable-diffusion-xl-base-1.0` and then tries to asses exactly this "path". 
It seems to be a regular case when `snapshot_download` and hence `hf_hub_download` is being called without the parameter `local_dir`.

`snapshot_download.py` (248:250) only performs `os.path.realpath()` when `local_dir` is not None:

```python
    if local_dir is not None:
        return str(os.path.realpath(local_dir))
    return snapshot_folder
```

My change in this PR now addresses this case and ensures that the real complete path is being used in `shutil.disk_usage().free` within `_check_disk_space()`.

HuggingFaceDocBuilderDev: The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_1692). All of your documentation changes will be reflected on that endpoint.
Wauplin: Hi @martinbrose thanks for looking into this! I think the problem is not with the confusion between absolute or relative paths. In theory `shutil.disk_usage()` is supposed to handle that correctly for us (I tried locally and  `shutil.disk_usage("path/to/folder")` has the same output as `shutil.disk_usage(os.path.realpath("path/to/folder"))` for me). It seems that the issue is more coming from the fact that the folder does not exist yet.

Here is what I think we should do:
- If `target_dir` does not exist, we check its parent directories recursively. If no parent folder exists, let's not raise a warning or exception. Not sure if this is possible but in any case the core "download logic" will take care of either creating the path or raise a proper exception
- Make sure that `_check_disk_space` never fails, no matter what. This method is supposed to help users but we don't want it to be a blocking in case something happens.

This would give an implementation looking like this:


```py
    target_dir = Path(target_dir)                         # format as `Path`
    for path in [target_dir] + list(target_dir.parents):  # first check target_dir, then each parents one by one
        try:
            target_dir_free = shutil.disk_usage(path).free
            if target_dir_free < expected_size:
                warnings.warn(
                    "Not enough free disk space to download the file. "
                    f"The expected file size is: {expected_size / 1e6:.2f} MB. "
                    f"The target location {target_dir} only has {target_dir_free / 1e6:.2f} MB free disk space."
                )
            return
        except OSError:                                   # raise on anything: file does not exist or space disk cannot be checked
            pass                                          # never raise. It's best to check parent folder instead
```

Corresponding tests could be extended (1 for missing folder, 1 for relative path). Do you want to take care of it @martinbrose ? Thanks in advance!
martinbrose: HI @Wauplin,

thanks for your reply. You seem to be right... just double checked myself.
I was convinced that this is the root problem. Weird.

I'll have a look tonight and update the PR tonight.
martinbrose: Hi @Wauplin,

your code recommendation makes sense... implemented and tested it.

I also added the two additional test cases. While I'm kind of confident with the test case for the non-existent path, I'm not sure I understand exactly the ask for a test case for relative path. Maybe I addressed it, maybe not. Please let me know.
martinbrose: This should fix the following issues:

- #1690 
- https://github.com/llm-efficiency-challenge/neurips_llm_efficiency_challenge/issues/26
- https://github.com/Lightning-AI/lit-gpt/issues/537
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/1692?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) Report
All modified lines are covered by tests :white_check_mark:
> Comparison is base [(`b171e02`)](https://app.codecov.io/gh/huggingface/huggingface_hub/commit/b171e02c3ea349bc4e7859c5ff2bb09b60121451?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) 82.41% compared to head [(`6513981`)](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/1692?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) 81.96%.
> Report is 2 commits behind head on main.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1692      +/-   ##
==========================================
- Coverage   82.41%   81.96%   -0.45%     
==========================================
  Files          62       62              
  Lines        7194     7198       +4     
==========================================
- Hits         5929     5900      -29     
- Misses       1265     1298      +33     
```


| [Files](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/1692?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) | Coverage Δ | |
|---|---|---|
| [src/huggingface\_hub/file\_download.py](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/1692?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9maWxlX2Rvd25sb2FkLnB5) | `85.36% <100.00%> (+0.11%)` | :arrow_up: |

... and [4 files with indirect coverage changes](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/1692/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/1692?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface).


Patch:
diff --git a/src/huggingface_hub/file_download.py b/src/huggingface_hub/file_download.py
index 9191f2d8..ca2e8ebf 100644
--- a/src/huggingface_hub/file_download.py
+++ b/src/huggingface_hub/file_download.py
@@ -972,17 +972,19 @@ def _check_disk_space(expected_size: int, target_dir: Union[str, Path]) -> None:
             The directory where the file will be stored after downloading.
     """
 
-    target_dir = str(target_dir)
-    target_dir_free = shutil.disk_usage(target_dir).free
-
-    has_enough_space = target_dir_free >= expected_size
-
-    if not has_enough_space:
-        warnings.warn(
-            "Not enough free disk space to download the file. "
-            f"The expected file size is: {expected_size / 1e6:.2f} MB. "
-            f"The target location {target_dir} only has {target_dir_free / 1e6:.2f} MB free disk space."
-        )
+    target_dir = Path(target_dir)  # format as `Path`
+    for path in [target_dir] + list(target_dir.parents):  # first check target_dir, then each parents one by one
+        try:
+            target_dir_free = shutil.disk_usage(path).free
+            if target_dir_free < expected_size:
+                warnings.warn(
+                    "Not enough free disk space to download the file. "
+                    f"The expected file size is: {expected_size / 1e6:.2f} MB. "
+                    f"The target location {target_dir} only has {target_dir_free / 1e6:.2f} MB free disk space."
+                )
+            return
+        except OSError:  # raise on anything: file does not exist or space disk cannot be checked
+            pass
 
 
 @validate_hf_hub_args


Test patch:
diff --git a/tests/test_file_download.py b/tests/test_file_download.py
index cf841a7c..8c90e07e 100644
--- a/tests/test_file_download.py
+++ b/tests/test_file_download.py
@@ -112,6 +112,21 @@ class TestDiskUsageWarning(unittest.TestCase):
             _check_disk_space(expected_size=self.expected_size, target_dir=disk_usage_mock)
             assert len(w) == 0
 
+    def test_disk_usage_warning_with_non_existent_path(self) -> None:
+        # Test for not existent (absolute) path
+        with warnings.catch_warnings(record=True) as w:
+            # Cause all warnings to always be triggered.
+            warnings.simplefilter("always")
+            _check_disk_space(expected_size=self.expected_size, target_dir="path/to/not_existent_path")
+            assert len(w) == 0
+
+        # Test for not existent (relative) path
+        with warnings.catch_warnings(record=True) as w:
+            # Cause all warnings to always be triggered.
+            warnings.simplefilter("always")
+            _check_disk_space(expected_size=self.expected_size, target_dir="/path/to/not_existent_path")
+            assert len(w) == 0
+
 
 @with_production_testing
 class CachedDownloadTests(unittest.TestCase):