Address failing _check_disk_space() when path doesn't exist yet

[martinbrose]

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:

    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 documentation is not available anymore as the PR was closed or merged.

[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:

    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 paths. Maybe I addressed it, maybe not. Please let me know.

[martinbrose]

This should fix the following issues:

• check_disk_space fails when the target directory does not exist #1690
• Error while dowloading 'checkpoints/openlm-research/open_llama_3b' llm-efficiency-challenge/neurips_llm_efficiency_challenge#26
• FileNotFoundError with vicuna-7b-v1.5 Lightning-AI/litgpt#537

[Wauplin]

Yes that's exactly how I was seeing it except that 1 path must be relative (let's say "path/to/not_existent_path") and the other one absolute ("/path/to/not_existent_path") to check that both of them doesn't raise an exception.

[Wauplin]

Suggested change

	def test_disk_usage_warning_with_non_existent_path(self) -> None:
	# Test for not existent 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="not_existent_path")
	assert len(w) == 0
	# Test for 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="./not_existent_path")
	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

[Wauplin]

Thanks for making the changes @martinbrose! Test looks good. I've updated it to have both an absolute and a relative paths but that's it. We are good to merge now :)

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (b171e02) 82.41% compared to head (6513981) 81.96%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            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	Coverage Δ
src/huggingface_hub/file_download.py	85.36% <100.00%> (+0.11%)	⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.