Skip to content

fix: skip redundant HfFileSystem().glob() calls in loader.py #4852

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danielhanchen merged 8 commits into from
Apr 6, 2026
Merged

fix: skip redundant HfFileSystem().glob() calls in loader.py #4852

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dac1b60
fix: skip redundant HfFileSystem().glob() calls in loader.py
rolandtannous Apr 5, 2026
c50591b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 5, 2026
30db8f3
Remove test file from main PR - moved to separate PR
danielhanchen Apr 5, 2026
b83d57f
Merge remote-tracking branch 'origin/main' into pr-4852-head
danielhanchen Apr 6, 2026
1d46c65
Harden HfFileSystem glob: fix Windows path splitting, add try/except
danielhanchen Apr 6, 2026
54ffa99
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 6, 2026
6025e9b
Remove redundant HfFileSystem().glob() call for remote repos
danielhanchen Apr 6, 2026
a21efff
Remove unused HfFileSystem import
danielhanchen Apr 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 10 additions & 18 deletions unsloth/models/loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
except:
# For older versions of huggingface_hub
from huggingface_hub.utils._token import get_token
from huggingface_hub import HfFileSystem
import importlib.util
from ..device_type import (
is_hip,
Expand Down Expand Up @@ -508,7 +507,7 @@ def from_pretrained(
model_type = model_types

# New transformers need to check manually.
if SUPPORTS_LLAMA32:
if SUPPORTS_LLAMA32 and is_model and is_peft:
Comment thread
rolandtannous marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The manual check for file existence using os.path.exists or HfFileSystem().glob() is redundant when guarded by is_model and is_peft. If both AutoConfig.from_pretrained and PeftConfig.from_pretrained succeeded (setting is_model and is_peft to True), it already confirms that both configuration files exist and are reachable for the given model_name.

Additionally, the both_exist variable set within this block (lines 508 and 517) is never checked again in this function. The only check for both_exist occurs at line 484, which is before this block executes. This means that for SUPPORTS_LLAMA32, the "mixed repo" error is currently never raised, making this entire block a no-op that only serves to potentially cause the network hangs you are fixing.

A cleaner solution would be to unify the logic at line 481 by removing the and not SUPPORTS_LLAMA32 condition. This would fix the hang, remove the redundancy, and ensure the error is correctly raised for all versions.

# Check if folder exists locally
if os.path.isdir(model_name):
exist_adapter_config = os.path.exists(
Expand All @@ -517,14 +516,10 @@ def from_pretrained(
exist_config = os.path.exists(os.path.join(model_name, "config.json"))
both_exist = exist_adapter_config and exist_config
else:
# Because HfFileSystem assumes linux paths, we need to set the path with forward slashes, even on Windows.
files = HfFileSystem(token = token).glob(f"{model_name}/*.json")
files = list(os.path.split(x)[-1] for x in files)
if (
sum(x == "adapter_config.json" or x == "config.json" for x in files)
>= 2
):
both_exist = True
# Both AutoConfig and PeftConfig loaded successfully from this
# remote repo, so both config.json and adapter_config.json
# definitely exist -- no need for an extra HfFileSystem network call.
both_exist = True

if not is_model and not is_peft:
error = autoconfig_error if autoconfig_error is not None else peft_error
Expand Down Expand Up @@ -1282,7 +1277,7 @@ def from_pretrained(
os.environ["UNSLOTH_DISABLE_STATIC_GENERATION"] = "1"

# New transformers need to check manually.
if SUPPORTS_LLAMA32:
if SUPPORTS_LLAMA32 and is_model and is_peft:
Comment thread
rolandtannous marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the check in FastLanguageModel.from_pretrained, this manual file check is redundant when is_model and is_peft are both True. Moreover, the both_exist variable updated here (lines 1284 and 1292) is not used anywhere after this block. The error check at line 1097 happens before this manual check, so the conflict is never actually flagged for Llama 3.2+ models.

Simplifying the logic at line 1095 to both_exist = is_model and is_peft (removing the version check) would be more efficient and correct.

# Check if folder exists locally
if os.path.isdir(model_name):
exist_adapter_config = os.path.exists(
Expand All @@ -1291,13 +1286,10 @@ def from_pretrained(
exist_config = os.path.exists(os.path.join(model_name, "config.json"))
both_exist = exist_adapter_config and exist_config
else:
files = HfFileSystem(token = token).glob(f"{model_name}/*.json")
files = list(os.path.split(x)[-1] for x in files)
if (
sum(x == "adapter_config.json" or x == "config.json" for x in files)
>= 2
):
both_exist = True
# Both AutoConfig and PeftConfig loaded successfully from this
# remote repo, so both config.json and adapter_config.json
# definitely exist -- no need for an extra HfFileSystem network call.
both_exist = True

if not is_model and not is_peft:
error = autoconfig_error if autoconfig_error is not None else peft_error
Expand Down
Loading