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 6 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
51 changes: 34 additions & 17 deletions unsloth/models/loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,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:

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

Copy link
Copy Markdown

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.

@danielhanchen danielhanchen Apr 6, 2026

Copy link
Copy Markdown
Member Author

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.

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

Copy link
Copy Markdown

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.

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

Copy link
Copy Markdown

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 +517,22 @@ 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
# HfFileSystem returns POSIX paths; use str.rsplit to extract
# filenames correctly on all platforms (os.path.split uses the
# OS separator which breaks on Windows).
try:
files = HfFileSystem(token = token).glob(f"{model_name}/*.json")
files = [x.rsplit("/", 1)[-1] for x in files]
if (
sum(
x == "adapter_config.json" or x == "config.json"
for x in files
)
>= 2
):
both_exist = True
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
except Exception:
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
pass

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using a bare except Exception: block is discouraged as it can mask unexpected errors, including keyboard interrupts or system-level exceptions. It is better to catch specific exceptions that are expected during the HfFileSystem operations.

@danielhanchen danielhanchen Apr 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

[8/13 reviewers] The except Exception: pass is too broad. It silently swallows MemoryError, AttributeError, TypeError, auth failures (RepositoryNotFoundError, GatedRepoError), and any other programming errors with zero logging. This makes failures very hard to diagnose.

However, this entire remote branch is unnecessary. When is_model=True and is_peft=True, both config.json (loaded by AutoConfig) and adapter_config.json (loaded by PeftConfig) are proven to exist. The HfFileSystem().glob() call is redundant information gathering.

Suggested change
pass
else:
# 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

This eliminates the network call entirely, removes the need for try/except, and is provably correct.


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 +1290,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:

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

Copy link
Copy Markdown

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.

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

Copy link
Copy Markdown

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 +1299,22 @@ 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
# HfFileSystem returns POSIX paths; use str.rsplit to extract
# filenames correctly on all platforms (os.path.split uses the
# OS separator which breaks on Windows).
try:
files = HfFileSystem(token = token).glob(f"{model_name}/*.json")
files = [x.rsplit("/", 1)[-1] for x in files]
if (
sum(
x == "adapter_config.json" or x == "config.json"
for x in files
)
>= 2
):
both_exist = True
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
except Exception:
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
pass

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using a bare except Exception: block is discouraged as it can mask unexpected errors, including keyboard interrupts or system-level exceptions. It is better to catch specific exceptions that are expected during the HfFileSystem operations.

@danielhanchen danielhanchen Apr 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

[8/13 reviewers] Same issue as above -- the except Exception: pass and the entire glob call are redundant. Since is_model=True and is_peft=True, both config files are proven to exist.

Suggested change
pass
else:
# 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