fix: address Codex review on offline GGUF tokenizer paths (#7481)
- Only rewrite Hub repo ids to cached snapshot dirs when offline - Copy tokenizer.model from cache offline in preserve_sentencepiece - Do not cache negative offline tokenizer.model probe results - Add regression tests for all three review items
This commit is contained in:
parent
8a0b7bb5b3
commit
e290ebd0ac
3 changed files with 117 additions and 21 deletions
|
|
@ -66,11 +66,25 @@ def _offline_env(monkeypatch, cache_root):
|
|||
monkeypatch.setenv("HF_HUB_CACHE", str(cache_root))
|
||||
|
||||
|
||||
def test_resolve_hub_repo_cached_file_finds_tokenizer_model(tmp_path, monkeypatch):
|
||||
snap = _write_gemma4_cache(tmp_path)
|
||||
(snap / "tokenizer.model").write_bytes(b"sp-model")
|
||||
_offline_env(monkeypatch, tmp_path)
|
||||
|
||||
got = L._resolve_hub_repo_cached_file(
|
||||
_REPO,
|
||||
"tokenizer.model",
|
||||
local_files_only=True,
|
||||
cache_dir=str(tmp_path),
|
||||
)
|
||||
assert got == str(snap / "tokenizer.model")
|
||||
|
||||
|
||||
def test_resolve_hub_repo_local_dir_from_cached_snapshot(tmp_path, monkeypatch):
|
||||
snap = _write_gemma4_cache(tmp_path)
|
||||
_offline_env(monkeypatch, tmp_path)
|
||||
|
||||
got = L._resolve_hub_repo_local_dir(_REPO, local_files_only = True, cache_dir = str(tmp_path))
|
||||
got = L._resolve_hub_repo_local_dir(_REPO, local_files_only=True, cache_dir=str(tmp_path))
|
||||
assert got == str(snap)
|
||||
|
||||
|
||||
|
|
@ -78,11 +92,55 @@ def test_hub_repo_or_local_path_prefers_snapshot_over_repo_id(tmp_path, monkeypa
|
|||
snap = _write_gemma4_cache(tmp_path)
|
||||
_offline_env(monkeypatch, tmp_path)
|
||||
|
||||
got = L._hub_repo_or_local_path(_REPO, local_files_only = True, cache_dir = str(tmp_path))
|
||||
got = L._hub_repo_or_local_path(_REPO, local_files_only=True, cache_dir=str(tmp_path))
|
||||
assert got == str(snap)
|
||||
assert got != _REPO
|
||||
|
||||
|
||||
def test_hub_repo_or_local_path_keeps_repo_id_online(tmp_path, monkeypatch):
|
||||
snap = _write_gemma4_cache(tmp_path)
|
||||
monkeypatch.delenv("HF_HUB_OFFLINE", raising=False)
|
||||
monkeypatch.delenv("TRANSFORMERS_OFFLINE", raising=False)
|
||||
monkeypatch.setenv("HF_HUB_CACHE", str(tmp_path))
|
||||
|
||||
got = L._hub_repo_or_local_path(_REPO, local_files_only=False, cache_dir=str(tmp_path))
|
||||
assert got == _REPO
|
||||
assert got != str(snap)
|
||||
|
||||
|
||||
def test_has_tokenizer_model_offline_does_not_cache_negative(tmp_path, monkeypatch):
|
||||
from unsloth.save import _TOKENIZER_MODEL_CACHE, _has_tokenizer_model
|
||||
|
||||
snap = _write_gemma4_cache(tmp_path)
|
||||
_offline_env(monkeypatch, tmp_path)
|
||||
_TOKENIZER_MODEL_CACHE.clear()
|
||||
|
||||
tok = SimpleNamespace(name_or_path=_REPO)
|
||||
assert _has_tokenizer_model(tok, token=None) is False
|
||||
assert _REPO not in _TOKENIZER_MODEL_CACHE
|
||||
|
||||
(snap / "tokenizer.model").write_bytes(b"sp-model")
|
||||
assert _has_tokenizer_model(tok, token=None) is True
|
||||
|
||||
|
||||
def test_preserve_sentencepiece_offline_copies_cached_model(tmp_path, monkeypatch):
|
||||
from unsloth.save import _TOKENIZER_MODEL_CACHE, _preserve_sentencepiece_tokenizer_assets
|
||||
|
||||
snap = _write_gemma4_cache(tmp_path)
|
||||
(snap / "tokenizer.model").write_bytes(b"cached-sp-model")
|
||||
_offline_env(monkeypatch, tmp_path)
|
||||
_TOKENIZER_MODEL_CACHE.clear()
|
||||
|
||||
save_dir = tmp_path / "export"
|
||||
save_dir.mkdir()
|
||||
(save_dir / "tokenizer_config.json").write_text("{}", encoding="utf-8")
|
||||
tok = SimpleNamespace(name_or_path=_REPO)
|
||||
|
||||
_preserve_sentencepiece_tokenizer_assets(tok, str(save_dir))
|
||||
|
||||
assert (save_dir / "tokenizer.model").read_bytes() == b"cached-sp-model"
|
||||
|
||||
|
||||
def test_load_pretrained_tokenizer_fast_passes_snapshot_not_repo_id(tmp_path, monkeypatch):
|
||||
snap = _write_gemma4_cache(tmp_path)
|
||||
_offline_env(monkeypatch, tmp_path)
|
||||
|
|
@ -111,12 +169,13 @@ def test_load_pretrained_tokenizer_fast_passes_snapshot_not_repo_id(tmp_path, mo
|
|||
|
||||
|
||||
def test_has_tokenizer_model_offline_skips_model_info(tmp_path, monkeypatch):
|
||||
from unsloth.save import _has_tokenizer_model
|
||||
from unsloth.save import _TOKENIZER_MODEL_CACHE, _has_tokenizer_model
|
||||
|
||||
_write_gemma4_cache(tmp_path)
|
||||
_offline_env(monkeypatch, tmp_path)
|
||||
_TOKENIZER_MODEL_CACHE.clear()
|
||||
|
||||
tok = SimpleNamespace(name_or_path = _REPO, tokenizer = None)
|
||||
tok = SimpleNamespace(name_or_path = _REPO)
|
||||
|
||||
with patch("huggingface_hub.HfApi.model_info") as model_info:
|
||||
model_info.side_effect = AssertionError("model_info must not run offline")
|
||||
|
|
|
|||
|
|
@ -1170,6 +1170,28 @@ def _resolve_hub_repo_local_dir(
|
|||
return None
|
||||
|
||||
|
||||
def _resolve_hub_repo_cached_file(
|
||||
repo_id,
|
||||
filename,
|
||||
*,
|
||||
token = None,
|
||||
cache_dir = None,
|
||||
local_files_only = False,
|
||||
):
|
||||
"""Return a cached file path under a Hub snapshot, or None if absent."""
|
||||
local_dir = _resolve_hub_repo_local_dir(
|
||||
repo_id,
|
||||
token = token,
|
||||
cache_dir = cache_dir,
|
||||
local_files_only = local_files_only,
|
||||
filenames = (filename,),
|
||||
)
|
||||
if local_dir is None:
|
||||
return None
|
||||
path = os.path.join(local_dir, filename)
|
||||
return path if os.path.isfile(path) else None
|
||||
|
||||
|
||||
def _hub_repo_or_local_path(
|
||||
repo_id,
|
||||
*,
|
||||
|
|
@ -1179,14 +1201,16 @@ def _hub_repo_or_local_path(
|
|||
filenames = None,
|
||||
):
|
||||
"""Prefer a cached snapshot path over a Hub repo id when offline or ``local_files_only``."""
|
||||
if isinstance(repo_id, str) and os.path.isdir(repo_id):
|
||||
return repo_id
|
||||
lfo = bool(local_files_only) or _env_says_offline()
|
||||
if not lfo and os.path.isdir(repo_id):
|
||||
if not lfo:
|
||||
return repo_id
|
||||
local_dir = _resolve_hub_repo_local_dir(
|
||||
repo_id,
|
||||
token = token,
|
||||
cache_dir = cache_dir,
|
||||
local_files_only = lfo,
|
||||
local_files_only = True,
|
||||
filenames = filenames
|
||||
or (
|
||||
"tokenizer_config.json",
|
||||
|
|
|
|||
|
|
@ -52,7 +52,7 @@ import traceback
|
|||
import psutil
|
||||
import re
|
||||
from transformers.models.llama.modeling_llama import logger
|
||||
from .models.loader_utils import get_model_name, _env_says_offline, _resolve_hub_repo_local_dir
|
||||
from .models.loader_utils import get_model_name, _env_says_offline, _resolve_hub_repo_cached_file
|
||||
from .models._utils import _convert_torchao_model
|
||||
from .ollama_template_mappers import OLLAMA_TEMPLATES, MODEL_TO_OLLAMA_TEMPLATE_MAPPER
|
||||
from transformers import ProcessorMixin, PreTrainedTokenizerBase
|
||||
|
|
@ -448,17 +448,16 @@ def _has_tokenizer_model(tokenizer, token = None):
|
|||
|
||||
# Offline: probe the local cache instead of model_info (issue #7481).
|
||||
if _env_says_offline():
|
||||
local_dir = _resolve_hub_repo_local_dir(
|
||||
cached_path = _resolve_hub_repo_cached_file(
|
||||
source,
|
||||
"tokenizer.model",
|
||||
token = token,
|
||||
local_files_only = True,
|
||||
filenames = ("tokenizer.model", "tokenizer.json", "tokenizer_config.json"),
|
||||
cache_dir = os.environ.get("HF_HUB_CACHE"),
|
||||
)
|
||||
if local_dir is not None:
|
||||
has_tokenizer_model = os.path.isfile(os.path.join(local_dir, "tokenizer.model"))
|
||||
_TOKENIZER_MODEL_CACHE[source] = has_tokenizer_model
|
||||
return has_tokenizer_model
|
||||
if cached_path is not None:
|
||||
_TOKENIZER_MODEL_CACHE[source] = True
|
||||
return True
|
||||
return False
|
||||
|
||||
try:
|
||||
|
|
@ -520,15 +519,29 @@ def _preserve_sentencepiece_tokenizer_assets(
|
|||
if os.path.isfile(local_path):
|
||||
downloaded_path = local_path
|
||||
else:
|
||||
from huggingface_hub import hf_hub_download
|
||||
try:
|
||||
downloaded_path = hf_hub_download(
|
||||
repo_id = source,
|
||||
filename = "tokenizer.model",
|
||||
cached_path = None
|
||||
if _env_says_offline():
|
||||
cached_path = _resolve_hub_repo_cached_file(
|
||||
source,
|
||||
"tokenizer.model",
|
||||
token = token,
|
||||
local_files_only = True,
|
||||
cache_dir = os.environ.get("HF_HUB_CACHE"),
|
||||
)
|
||||
except Exception:
|
||||
downloaded_path = None
|
||||
if cached_path is not None:
|
||||
downloaded_path = cached_path
|
||||
else:
|
||||
from huggingface_hub import hf_hub_download
|
||||
try:
|
||||
downloaded_path = hf_hub_download(
|
||||
repo_id = source,
|
||||
filename = "tokenizer.model",
|
||||
token = token,
|
||||
local_files_only = _env_says_offline(),
|
||||
cache_dir = os.environ.get("HF_HUB_CACHE"),
|
||||
)
|
||||
except Exception:
|
||||
downloaded_path = None
|
||||
|
||||
if not os.path.isfile(tokenizer_model) and downloaded_path is not None:
|
||||
shutil.copy2(downloaded_path, tokenizer_model)
|
||||
|
|
@ -3808,7 +3821,7 @@ def unsloth_convert_lora_to_ggml_and_save_locally(
|
|||
return _unsloth_save_lora_gguf(self, tokenizer, save_directory, outtype = outtype)
|
||||
|
||||
|
||||
from .models.loader_utils import get_model_name, _env_says_offline, _resolve_hub_repo_local_dir
|
||||
from .models.loader_utils import get_model_name, _env_says_offline, _resolve_hub_repo_cached_file
|
||||
from unsloth_zoo.saving_utils import (
|
||||
merge_and_overwrite_lora,
|
||||
prepare_saving,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue