Skip to content

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

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

Closed
danielhanchen wants to merge 8 commits into from
Closed

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

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:
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

high

The optimization to guard the manual check with is_model and is_peft is a good improvement for performance. However, there is a logic bug in the surrounding code: the both_exist flag set within this block (at line 527) is never checked or used to raise an error for Llama 3.2+ models. This means the conflict detection is currently non-functional for these models.

Furthermore, if is_model and is_peft are both True, it already indicates that both AutoConfig and PeftConfig were successfully loaded for the same model_name, which confirms the conflict. You could potentially avoid the expensive HfFileSystem().glob() call entirely by raising the RuntimeError immediately when both are True, which would also fix the bug and prevent hangs in conflict scenarios.

Copy link
Copy Markdown
Member Author

@danielhanchen danielhanchen Apr 6, 2026

Choose a reason for hiding this comment

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

[5/13 reviewers] Pre-existing issue (not introduced by this PR): both_exist is dead code after this block.

The flow is:

  1. L490: both_exist = (is_model and is_peft) and not SUPPORTS_LLAMA32 -- always False when SUPPORTS_LLAMA32=True
  2. L493: if both_exist: raise RuntimeError(...) -- fires BEFORE this block, only on old transformers
  3. L511-535: This block sets both_exist but it is NEVER READ again afterward

The RuntimeError for "both configs exist" effectively never fires on modern transformers. This is a pre-existing silent safety bypass. The fix would be to add if both_exist: raise RuntimeError(...) after this block, but that changes behavior and should be a separate PR.

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

medium

The condition is_model and is_peft is checked here and again in the if not is_model and not is_peft block on line 525. This logic is duplicated in the from_pretrained method for both FastLanguageModel and FastModel. Consider refactoring this into a helper function to improve maintainability and reduce code duplication.

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

medium

The condition SUPPORTS_LLAMA32 and is_model and is_peft is repeated in multiple places. Consider refactoring this into a helper function or a boolean variable to improve maintainability and readability.

# 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
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed

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:
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

high

Similar to the issue in FastLanguageModel.from_pretrained, the both_exist flag set at line 1300 is never subsequently checked, rendering the manual conflict detection for Llama 3.2+ models ineffective. Since is_model and is_peft being True already confirms that both configurations are present and loadable, consider raising the conflict error directly to avoid the redundant and potentially slow HfFileSystem().glob() call.

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

medium

The condition SUPPORTS_LLAMA32 and is_model and is_peft is repeated in multiple places. Consider refactoring this into a helper function or a boolean variable to improve maintainability and readability.

# 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
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed

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