Use extended path on Windows when downloading to local dir #2378

mlinke-ai · 2024-07-06T17:30:16Z

Change the path of the local dir to an extended path by prepending "\?" to the absolute path, when the absolute path is longer than 255 characters on Windows.

This addresses Issue #2374.

Also fixed a small typo.

Change the path of the local dir to an extended path by prepending "\\?\" to the absolute path, when the absolute path is longer than 255 characters on Windows. Also fixed a small typo.

Wauplin

Thanks for opening this PR @mlinke-ai! I left a comment to make sure implementation is as robust as possible. Let me know what you think :)

Wauplin · 2024-07-08T14:07:23Z

src/huggingface_hub/file_download.py

+    # Some Windows versions do not allow for paths longer than 255 characters.
+    # In this case, we must specify it as an extended path by using the "\\?\" prefix.
+    if os.name == "nt" and len(os.path.abspath(local_dir)) > 255:
+        local_dir = "\\\\?\\" + os.path.abspath(local_dir)
    local_dir = Path(local_dir)
    paths = get_local_download_paths(local_dir=local_dir, filename=filename)


There might be cases where os.path.abspath(local_dir) is smaller than 255 characters but the lock file or file path not. What I would do to be safer is to move this logic to get_local_download_paths internals itself. If lock_path is >= 255 characters, then you should prefixed all paths with \\?\ (if not already the case). In practice lock_path is "guaranteed" to be the longest path so if this one is ok, all of them are.

I agree, having the path handling in one location is a much better approach. I have pushed some changes following your recommendation.

Maybe we also need some tests for get_local_download_paths()?

Change the path of the local dir to an extended path by prepending "\\?\" to the absolute path, when the absolute path is longer than 255 characters on Windows. Also fixed a small typo.

On Windows we check the length of `lock_path` and if it is longer than 255 characters we prepend the `\\?\` prefix to all paths if it does not already exist. We only need to check the length of `lock_path` because it is guaranteed to be the longest path.

…uggingface_hub into download-using-extended-path

HuggingFaceDocBuilderDev · 2024-07-11T10:14:40Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Wauplin

Thanks for the changes @mlinke-ai! Adding tests would be awesome. You can do that in ./tests/test_local_folder.py. The test should differentiate the expected output depending on if it runs on Windows or not.

Wauplin · 2024-07-11T10:11:41Z

src/huggingface_hub/file_download.py

+    # Some Windows versions do not allow for paths longer than 255 characters.
+    # In this case, we must specify it as an extended path by using the "\\?\" prefix.
+    if os.name == "nt" and len(os.path.abspath(local_dir)) > 255:
+        local_dir = "\\\\?\\" + os.path.abspath(local_dir)


This can now be removed! :)

Wauplin · 2024-07-11T10:14:21Z

src/huggingface_hub/_local_folder.py

+    # Some Windows versions do not allow for paths longer than 255 characters.
+    # In this case, we must specify it as an extended path by using the "\\?\" prefix
+    if os.name == "nt":
+        if len(os.path.abspath(lock_path)) > 255:


Suggested change

if len(os.path.abspath(lock_path)) > 255:

if len(os.path.abspath(lock_path)) > 255 and not str(local_dir).startwith("\\\\?\\"):

Let's check the length of lock_path and the prefix of local_dir only once. If the condition above is true, then all 3 paths must be changed. The logic is the same but we about the 3 xxx_path = xxx_path if ... else Path(...) below.

What about we change the order of the tests:

if os.name == "nt" and not str(local_dir).startswith("\\\\?\\"): if len(os.path.abspath(lock_path)) > 255: file_path = Path("\\\\?\\" + os.path.abspath(file_path)) lock_path = Path("\\\\?\\" + os.path.abspath(lock_path)) metadata_path = Path("\\\\?\\" + os.path.abspath(metadata_path))

In this case we only need to check the length of the path if the user has not already specified the \\?\ prefix. Because we only need to care about the length if the prefix is not specified.

Both are correct so feel free to put the order you feel is the more appropriate :) Much cleaner like this in any case!

(EDIT: slight preference to either have a single if statement or have two if statement with the first one been if os.name == "nt" alone)

* Fix token=False not respected in file download * lint

LysandreJik · 2024-07-11T13:39:10Z

src/huggingface_hub/file_download.py

+    # Some Windows versions do not allow for paths longer than 255 characters.
+    # In this case, we must specify it as an extended path by using the "\\?\" prefix.
+    if os.name == "nt" and len(os.path.abspath(local_dir)) > 255:
+        local_dir = "\\\\?\\" + os.path.abspath(local_dir)


…el` (#2373) * Handle shared layers in save_torch_state_dict + save_torch_model + some helpers * fix pytest rerun * more reruns

…et/Space) (#2333) * First draft to support `expand` parameter for models * add expand support for dataset * add expand support for Space

Change the path of the local dir to an extended path by prepending "\\?\" to the absolute path, when the absolute path is longer than 255 characters on Windows. Also fixed a small typo.

On Windows we check the length of `lock_path` and if it is longer than 255 characters we prepend the `\\?\` prefix to all paths if it does not already exist. We only need to check the length of `lock_path` because it is guaranteed to be the longest path.

Change the path of the local dir to an extended path by prepending "\\?\" to the absolute path, when the absolute path is longer than 255 characters on Windows. Also fixed a small typo.

…uggingface_hub into download-using-extended-path

tests/test_local_folder.py

The test now shows up a `skipped` if executed on a non-Windows machine Co-authored-by: Lucain <lucainp@gmail.com>

Wauplin

Thanks for iterating on it @mlinke-ai! Looks good to me now :) Let's wait for the CI to pass and it should be good to merge!

EDIT: code quality seems to fail. Could you install pip install -e ".[dev]" locally and then run make style to fix them. To check everything is fixed, you can run make quality. Then commit the changes and you're good!

mlinke-ai · 2024-07-11T16:25:17Z

No problem.

Please excuse the back and forth; this is actually my first PR.

Wauplin · 2024-07-11T17:18:50Z

No problem at all! Going back and forth is the essence of open-source collaboration :) Glad to know your first PR is on huggingface_hub's repo 🤗

Wauplin · 2024-07-11T17:19:12Z

Failing tests are unrelated to this PR so it's good to merge! 🚀

Use extended path on Windows when downloading to local dir

f2d2248

Change the path of the local dir to an extended path by prepending "\\?\" to the absolute path, when the absolute path is longer than 255 characters on Windows. Also fixed a small typo.

Wauplin reviewed Jul 8, 2024

View reviewed changes

mlinke-ai and others added 4 commits July 10, 2024 17:58

Use extended path on Windows when downloading to local dir

ec9f0b0

Change the path of the local dir to an extended path by prepending "\\?\" to the absolute path, when the absolute path is longer than 255 characters on Windows. Also fixed a small typo.

Merge branch 'download-using-extended-path' of github.com:mlinke-ai/h…

4967e56

…uggingface_hub into download-using-extended-path

Merge branch 'main' into download-using-extended-path

0b30635

Wauplin reviewed Jul 11, 2024

View reviewed changes

qgallouedec and others added 2 commits July 11, 2024 13:07

safetensors[torch] (#2371)

51d03b2

Fix token=False not respected in file download (#2386)

d624b46

* Fix token=False not respected in file download * lint

LysandreJik approved these changes Jul 11, 2024

View reviewed changes

Wauplin and others added 8 commits July 11, 2024 16:08

Handle shared layers in save_torch_state_dict + add `save_torch_mod…

dfe72d0

…el` (#2373) * Handle shared layers in save_torch_state_dict + save_torch_model + some helpers * fix pytest rerun * more reruns

Support expand parameter in xxx_info and list_xxxs (model/datas…

4af0294

…et/Space) (#2333) * First draft to support `expand` parameter for models * add expand support for dataset * add expand support for Space

Use extended path on Windows when downloading to local dir

c2d69c2

Change the path of the local dir to an extended path by prepending "\\?\" to the absolute path, when the absolute path is longer than 255 characters on Windows. Also fixed a small typo.

Use extended path on Windows when downloading to local dir

4768739

Change the path of the local dir to an extended path by prepending "\\?\" to the absolute path, when the absolute path is longer than 255 characters on Windows. Also fixed a small typo.

Removed old path handling

f494887

Reorder path check; add tests

fceb585

Merge branch 'download-using-extended-path' of github.com:mlinke-ai/h…

78575a8

…uggingface_hub into download-using-extended-path

Wauplin reviewed Jul 11, 2024

View reviewed changes

tests/test_local_folder.py Outdated Show resolved Hide resolved

mlinke-ai and others added 2 commits July 11, 2024 18:17

Skip test if opn Windows

1302f1c

The test now shows up a `skipped` if executed on a non-Windows machine Co-authored-by: Lucain <lucainp@gmail.com>

Fix indentation for test_local_folder.py

de3776e

Wauplin approved these changes Jul 11, 2024

View reviewed changes

Fix code style

a095d27

Wauplin merged commit 7fd7fcf into huggingface:main Jul 11, 2024
11 of 14 checks passed

Wauplin mentioned this pull request Jul 11, 2024

huggingface-cli fails to download model because of missing .incomplete file #2374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use extended path on Windows when downloading to local dir #2378

Use extended path on Windows when downloading to local dir #2378

mlinke-ai commented Jul 6, 2024

Wauplin left a comment

Wauplin Jul 8, 2024

mlinke-ai Jul 10, 2024

HuggingFaceDocBuilderDev commented Jul 11, 2024

Wauplin left a comment

Wauplin Jul 11, 2024

LysandreJik Jul 11, 2024

Wauplin Jul 11, 2024

mlinke-ai Jul 11, 2024

Wauplin Jul 11, 2024 •

edited

Loading

LysandreJik Jul 11, 2024

Wauplin left a comment •

edited

Loading

mlinke-ai commented Jul 11, 2024

Wauplin commented Jul 11, 2024

Wauplin commented Jul 11, 2024

	if len(os.path.abspath(lock_path)) > 255:
	if len(os.path.abspath(lock_path)) > 255 and not str(local_dir).startwith("\\\\?\\"):

Use extended path on Windows when downloading to local dir #2378

Use extended path on Windows when downloading to local dir #2378

Conversation

mlinke-ai commented Jul 6, 2024

Wauplin left a comment

Choose a reason for hiding this comment

Wauplin Jul 8, 2024

Choose a reason for hiding this comment

mlinke-ai Jul 10, 2024

Choose a reason for hiding this comment

HuggingFaceDocBuilderDev commented Jul 11, 2024

Wauplin left a comment

Choose a reason for hiding this comment

Wauplin Jul 11, 2024

Choose a reason for hiding this comment

LysandreJik Jul 11, 2024

Choose a reason for hiding this comment

Wauplin Jul 11, 2024

Choose a reason for hiding this comment

mlinke-ai Jul 11, 2024

Choose a reason for hiding this comment

Wauplin Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

LysandreJik Jul 11, 2024

Choose a reason for hiding this comment

Wauplin left a comment • edited Loading

Choose a reason for hiding this comment

mlinke-ai commented Jul 11, 2024

Wauplin commented Jul 11, 2024

Wauplin commented Jul 11, 2024

Wauplin Jul 11, 2024 •

edited

Loading

Wauplin left a comment •

edited

Loading