Offline: detect an unreachable hub, not just dead DNS - #7591
Conversation
Loading an already-downloaded model with no internet took 11 minutes. The offline guard only checked whether huggingface.co resolved, so the common offline shapes where DNS still answers (WAN down behind a live router, captive portal, stale DNS cache) were treated as online and every hub call burned its full retry backoff. Two fixes: - Escalate from the DNS check to the bounded, proxy-aware reachability probe already used by export, memoised for 60s and opt-outable with UNSLOTH_OFFLINE_PROBE=0. - Force offline in-process, not just via env vars. huggingface_hub and transformers read their offline constants at import and hub sessions cache a non-offline adapter, so setting the env mid-process left the calls retrying anyway. Also guards the metadata routes that had none (/models/config, /models/check-vision, /picker/chat-template, the per-request vision probe) and applies the same detection in the training worker. Measured on a cached GGUF repo with the endpoint blackholed: POST /inference/load 686s -> 4s GET /models/config 378s -> 0s GET /models/check-vision 28s -> 0s Online is unchanged: reachable endpoints skip the guard entirely, and a fresh download still resolves, downloads and loads normally.
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adbf88d8b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cached = _hf_reachability | ||
| if cached is not None and time.monotonic() - cached[0] < _HF_REACHABILITY_TTL_S: | ||
| return cached[1] |
There was a problem hiding this comment.
When the endpoint was reachable during an initial download and the WAN is then disconnected while DNS continues resolving, the cached False result is reused for 60 seconds. An immediate reload—the exact download-then-go-offline workflow this change targets—therefore skips the egress probe and can still enter the full Hugging Face retry backoff. Cache only unreachable results, or invalidate/re-probe reachable results before a guarded load.
Useful? React with 👍 / 👎.
| if _probe_dns_dead(): | ||
| return True |
There was a problem hiding this comment.
When HF_ENDPOINT points to a reachable mirror but huggingface.co does not resolve—for example on a restricted corporate network—this hard-coded DNS check returns early and never runs the endpoint-aware probe. The newly guarded config, vision, and template routes are then forced offline and cannot fetch uncached metadata from the working mirror. The preliminary DNS check must use the configured endpoint or be removed in favor of hf_endpoint_unreachable().
Useful? React with 👍 / 👎.
|
|
||
| if not hf_probe_disabled() and hf_endpoint_unreachable(): |
There was a problem hiding this comment.
When the startup HEAD probe receives a transient 502, 503, or 504, hf_endpoint_unreachable() classifies that HTTP response as unreachable and this branch permanently sets HF_HUB_OFFLINE, TRANSFORMERS_OFFLINE, and HF_DATASETS_OFFLINE for the worker. A brief Hub or proxy outage at startup therefore prevents every uncached model or dataset download for the entire training job even if the service immediately recovers; only connection failures should set lifetime offline flags, or the worker must re-probe before remote loads.
Useful? React with 👍 / 👎.
| # Guarded: this runs per request, so an unreachable hub would re-pay its retry | ||
| # backoff on every image/audio call. | ||
| with _hf_offline_if_unreachable(): | ||
| return bool(is_vision_model(load_path, hf_token = os.environ.get("HF_TOKEN"))) |
There was a problem hiding this comment.
During an image or audio auto-switch, target_id is guaranteed by _resolve_and_switch to be a concrete local path, and is_vision_model() returns from its local-GGUF/mmproj filesystem branch before any Hub access. Wrapping that check nevertheless adds a DNS lookup and potentially the full four-second HEAD timeout to the request when the WAN is unavailable but DNS still resolves, turning an otherwise immediate local capability check into a multi-second stall without preventing any retry.
Useful? React with 👍 / 👎.
|
Thanks, all four were real. I reproduced each one against the branch before changing anything, and re-measured after.
On the memo window, I did not go with "cache only unreachable results". Both directions are unsafe to hold: a stale On the gateway errors, I kept the default as-is so the scoped export callers are unchanged, and only the training worker (which sets flags for the whole job) opts into the strict mode. A downed hub should still send a single scoped operation to the cache. On Verification after the fixes:
New regression tests cover each finding: endpoint host parsing, DNS pre-check following |
…y mode Four issues raised on the first commit, all reproduced before fixing: - The DNS pre-check hardcoded huggingface.co, so a reachable HF_ENDPOINT mirror was forced offline whenever huggingface.co did not resolve. It now follows the configured endpoint. - The reachability verdict was memoised for 60s, and a stale "reachable" hid the user pulling the plug right after a download, which is the exact workflow this fix targets. Window is now 5s in both directions: long enough to dedupe the probes within one load, short enough that neither direction goes stale. - hf_endpoint_unreachable counts 502/503/504 as offline, and the training worker used it to set flags for the whole job, so a momentary hub blip blocked every download for the rest of the run. Added gateway_errors_offline=False for callers setting lifetime flags; scoped callers keep the existing behaviour. - Dropped the guard from _target_is_vision. The resolver only yields local paths there, so it returns from the mmproj filesystem branch without touching the hub, and the probe only added latency per request. Verified unchanged offline: load 686s -> 5s, /models/config 378s -> 0s, /models/check-vision 28s -> 0s.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ec30c54d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _t = _threading.Thread(target = _probe, daemon = True) | ||
| _t.start() | ||
| _t.join(2.0) | ||
| if _result[0] is False: |
There was a problem hiding this comment.
When a training worker runs with HF_ENDPOINT pointing at a reachable mirror but huggingface.co itself does not resolve, the hard-coded DNS probe above sets _result[0] to True, so this if _result[0] is False block skips the endpoint-aware hf_endpoint_unreachable() probe and the worker then sets lifetime HF_HUB_OFFLINE/HF_DATASETS_OFFLINE flags. That prevents model or dataset downloads from the configured mirror even though it is reachable; use the configured endpoint for the DNS precheck or defer to the endpoint probe before setting permanent offline flags.
Useful? React with 👍 / 👎.
| # Offline the guard keeps this on the HF cache instead of retrying the hub. | ||
| from core.inference.llama_cpp import _hf_offline_if_unreachable | ||
|
|
||
| with _hf_offline_if_unreachable(): |
There was a problem hiding this comment.
When model_name is a local GGUF/path, is_vision_model() returns from its filesystem/mmproj branch before any Hub access, but this new unconditional wrapper still runs the HF reachability probe first. In the offline shape where DNS still resolves, every local /models/check-vision request waits for the bounded HEAD timeout even though no Hub retry is being avoided; only wrap non-local model IDs.
Useful? React with 👍 / 👎.
| from core.inference.llama_cpp import _hf_offline_if_unreachable | ||
|
|
||
| def _read(): | ||
| with _hf_offline_if_unreachable(): |
There was a problem hiding this comment.
When model_name is a local path or GGUF, read_default_chat_template() stays on the filesystem and reads sidecar files or GGUF metadata, but this new wrapper still runs the HF reachability probe first. In WAN-down setups where DNS continues to resolve, opening the chat-template picker for a local model waits for the HEAD timeout even though no Hub call would have happened; guard only remote repo IDs or move the guard into the remote branch.
Useful? React with 👍 / 👎.
| DNS alone misses the common offline shapes (WAN down behind a live router, captive | ||
| portal, stale DNS cache), leaving every hub call to burn its full retry backoff. | ||
| """ | ||
| if _probe_dns_dead(): |
There was a problem hiding this comment.
When HTTPS_PROXY/HTTP_PROXY is the only path that can resolve and reach the Hub host, local DNS can fail while the proxy-aware urllib probe would still succeed. The fresh case is proxy-only DNS: this early return bypasses hf_unreachable() entirely and forces the newly guarded routes into cache-only mode, so uncached metadata/downloads fail despite working egress through the proxy; let the endpoint probe run before declaring the Hub offline.
Useful? React with 👍 / 👎.
…ocal skip All four reproduced before fixing, and re-measured after. - Proxy-only egress was declared offline. With HTTP(S)_PROXY set, the proxy resolves the hub host, so a failing local lookup says nothing. The DNS shortcut now stands down whenever a proxy applies (and honours NO_PROXY), letting the proxy-aware probe decide. Measured: endpoint probe reachable through the proxy while the guard still forced offline. - The training worker kept its own inline probe hardcoded to huggingface.co, so a reachable HF_ENDPOINT mirror set lifetime offline flags. It now uses the shared endpoint- and proxy-aware helper. - /models/check-vision, /models/config and /picker/chat-template ran the probe even for local paths, which never reach the hub. Measured 0.9s of pure latency per request; now skipped via _hf_offline_if_unreachable_for. DNS/endpoint/proxy helpers now live in utils.utils so llama_cpp and the training worker share one implementation instead of three copies. The static pin in test_offline_inference_parent moved with the probe: the worker block must delegate to the shared helper and must not hardcode a host, and the daemon-thread/no-setdefaulttimeout property is pinned on dns_host_dead where it now lives. Offline path unchanged: load 686s -> 6s, /models/config 378s -> 0s, and a local-path vision check is back to 0s.
|
Second round, all four valid again. Same approach: reproduce against the branch first, fix, then re-measure.
The proxy one was the interesting catch. My first attempt to reproduce it failed because The worker finding was a genuine miss on my side: I made the DNS check endpoint-aware in For the local-path findings I added One thing worth flagging: this refactor broke Verification:
|
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26d205b910
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| from utils.transformers_version import hf_endpoint_unreachable | ||
| unreachable = hf_endpoint_unreachable(timeout) |
There was a problem hiding this comment.
When a reachable Hub endpoint or corporate proxy takes more than the probe's three-second timeout to answer, hf_endpoint_unreachable() returns True, and this new call caches that verdict and makes the inference/model-loading guards force local-only mode. An uncached remote model then fails immediately instead of allowing the real Hub request to complete or retry, so slow connections are treated the same as no egress. A probe timeout should remain inconclusive/fail open rather than quarantine the load.
Useful? React with 👍 / 👎.
| if hf_dns_dead(): | ||
| return True | ||
| return hf_unreachable() |
There was a problem hiding this comment.
When HF_ENDPOINT is present but empty or whitespace-only, hf_dns_dead() treats it as the default Hugging Face host, but this call reaches hf_endpoint_unreachable(), which reads the raw value and probes the invalid URL https://. The resulting exception is classified as unreachable, so every uncached remote inference/model load is forced into local-only mode even though the default Hub is available. Use the same normalized endpoint URL for both stages.
Useful? React with 👍 / 👎.
| def _probe() -> None: | ||
| import socket as _socket | ||
| try: | ||
| _socket.gethostbyname(host) | ||
| result[0] = False | ||
| except Exception: | ||
| result[0] = True |
There was a problem hiding this comment.
When a custom HF_ENDPOINT has only an AAAA record or uses an IPv6 literal and no proxy applies, socket.gethostbyname() fails because it performs IPv4-only resolution. hf_dns_dead() therefore returns early without running the proxy-aware HTTP probe, forcing a reachable IPv6 Hub into offline mode and preventing uncached models from loading. Resolve with getaddrinfo() so either address family counts as reachable.
Useful? React with 👍 / 👎.
- A reachable endpoint that answers slower than the probe deadline was classified offline, so an uncached load failed instead of merely being slow. A clean socket timeout is now resolved with a bounded TCP connect: a loaded server still completes the handshake, a blackholed route does not. A refused connection counts as egress. - An empty or whitespace HF_ENDPOINT made the DNS shortcut fall back to the default hub while the HTTP probe probed "https://" and reported offline. Both stages now share one normaliser. - dns_host_dead used gethostbyname, which is IPv4-only and called an AAAA-only mirror or an IPv6 literal dead. It now uses getaddrinfo. A hang past the deadline still counts as unreachable: the real hub calls would hang the same way, so cache-only is the useful answer there. That distinction is what test_hung_probe_is_bounded pins, and it caught an earlier version of this change that treated every deadline overrun as inconclusive. Verified: slow endpoint reachable, blackholed route still offline, blank endpoint falls back, IPv6 literal resolves. Offline path unchanged, load 686s -> 9s and /models/config 378s -> 0s, chat still answers.
|
Round 3. All three were valid, and one of them exposed an overreach in my own first attempt at the fix.
On the slow-link one, the fix is not "treat every deadline overrun as inconclusive". I tried that first and it silently broke the case this PR exists for: a blackholed route also times out, so the blackhole test went from offline to reachable and the whole guard stopped engaging. The real discriminator is TCP: a loaded server still completes the handshake in milliseconds, a blackholed route does not. So a clean socket timeout now triggers a bounded Worth calling out that my first attempt also flipped the hung-probe case to reachable, and The blank-endpoint one was a straightforward inconsistency: Verification:
The DNS fixtures also had to move from patching |
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59929802c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| force_ctx = force_hf_offline() | ||
| force_ctx.__enter__() |
There was a problem hiding this comment.
When Studio handles overlapping remote metadata/load requests while the Hub is unreachable, the first guard sets HF_HUB_OFFLINE, causing every later _hf_offline_if_unreachable() call to take the early no-op branch instead of entering this ref-counted context. If the first request finishes while another is still resolving multiple Hub files, its exit restores the constants and sessions, and the remaining request resumes the long online retry path this change is intended to prevent. Each internally forced guard must hold its own reference until its operation exits, while still distinguishing a user-supplied offline variable.
Useful? React with 👍 / 👎.
| from utils.utils import hf_tcp_reachable | ||
| return not hf_tcp_reachable(min(timeout, 2.0), endpoint) |
There was a problem hiding this comment.
When the HEAD request times out through a configured proxy, this fallback calls hf_tcp_reachable, whose hf_connect_target deliberately connects only to the proxy host. An available proxy whose upstream route to Hugging Face is blackholed therefore returns True, so hf_endpoint_unreachable reports the Hub as reachable and all guarded Studio handlers still incur the full Hub retry delay. The timeout fallback needs to verify the proxy can reach the endpoint, not merely that the proxy accepts TCP connections.
Useful? React with 👍 / 👎.
- Overlapping requests lost offline mid-flight. A later guard saw the HF_HUB_OFFLINE that an earlier one had set and took the no-op branch, so when the earlier guard exited it restored the constants and sessions while the later request was still resolving hub files, dropping it back onto the retry path. Each guard now holds its own reference on the refcounted force_hf_offline window. A user-supplied offline variable is still left untouched, told apart via force_hf_offline_active(). - The socket-timeout fallback trusted a TCP handshake to the proxy, which only proves the proxy is up, not that it can reach the hub. A live proxy with a blackholed upstream therefore read as reachable. With a proxy configured the timeout now stays unreachable; the TCP check is only evidence when connecting to the endpoint directly. Verified: second guard engages and offline survives the first guard's exit, state fully restored after both; dead-upstream proxy reads unreachable while a slow direct endpoint still reads reachable; 9 concurrent metadata requests against an unreachable hub all return 200 in 5.1s total.
|
Round 4. Both valid, and the concurrency one is the sharpest catch so far. Concurrent guards. Reproduced with two overlapping guards: the second took the no-op branch ( Proxy TCP. Also correct, and it undercut my own round-3 fix. Reproduced with a listener that accepts TCP and never replies, standing in for a live proxy with a dead upstream: the probe reported the hub reachable. The TCP handshake is only evidence when we connect to the endpoint directly, so with a proxy configured a socket timeout now stays unreachable. The direct slow-endpoint case from round 3 still reports reachable, so that fix is intact. One correction to my own testing: my first end-to-end concurrency run appeared to hang for 40 minutes. That was my shell, not the code. A bare Verification:
|
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93dafbd429
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Each probe below can reach the hub, so the guard wraps the whole handler: | ||
| # offline they must all resolve from the HF cache instead of retrying. Local | ||
| # paths stay on disk, so they skip the probe entirely. | ||
| with _hf_offline_if_unreachable_for(model_name): |
There was a problem hiding this comment.
When egress is available only through ALL_PROXY and direct DNS is blocked, this new guard incorrectly forces the handler offline: hf_proxy_configured() recognizes the all entry and skips the DNS shortcut, but hf_endpoint_unreachable() probes with urllib.request.urlopen, which does not apply an ALL_PROXY-only setting and therefore fails its direct lookup. The Hugging Face client can still use that proxy, so uncached remote model configuration requests that previously worked now fail as cache-only; the reachability request should use the same proxy transport as the Hub client or map ALL_PROXY to the endpoint scheme.
Useful? React with 👍 / 👎.
| ours = force_hf_offline_active() | ||
| # A user-set offline var is theirs: don't probe it, don't touch it. | ||
| if "HF_HUB_OFFLINE" in os.environ and not ours: |
There was a problem hiding this comment.
When a user explicitly sets only TRANSFORMERS_OFFLINE=1, this check does not recognize their offline request and _hf_unreachable() now performs an HTTP HEAD probe before entering the guarded operation. That introduces network traffic in an explicitly offline process (the shared hf_env_offline() already treats either offline variable as authoritative); skip the reachability probe when either truthy offline variable is set.
Useful? React with 👍 / 👎.
…OFFLINE Resolve the hub proxy the way requests does (scheme-specific, then all_proxy, NO_PROXY wins) and issue the reachability HEAD through it. urllib ignores all_proxy, so a proxy-only setup failed the probe's direct lookup and was called offline while real hub calls would have succeeded. Skip the probe when TRANSFORMERS_OFFLINE alone is truthy: that is still an offline request, and the hub does not read it. Engage the guard directly instead of putting a DNS lookup and a HEAD in an explicitly offline process. HF_HUB_OFFLINE=0 remains an explicit stay-online opt-out.
|
Round 5, both comments checked against a real forward proxy on loopback rather than mocks. Both were correct. Probe through ALL_PROXY (P1). Confirmed. Fixed by adding Honor TRANSFORMERS_OFFLINE before probing (P2). Confirmed. The guard tested It now engages the guard directly when Ten regression tests added ( |
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2249ea0f97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ours = force_hf_offline_active() | ||
| # A user-set HF_HUB_OFFLINE is theirs: don't probe it, don't touch it. Membership, not | ||
| # truthiness, so HF_HUB_OFFLINE=0 stays an explicit "stay online" opt-out. | ||
| if "HF_HUB_OFFLINE" in os.environ and not ours: | ||
| yield False |
There was a problem hiding this comment.
When two guards start concurrently, one thread can read ours == False, pause, and then observe HF_HUB_OFFLINE after the other thread has entered force_hf_offline(). This branch consequently treats the variable as user-owned and yields without acquiring its own reference; if the other guard exits first, it restores the online constants and sessions while this operation is still running, sending an offline cache load back into Hub retries. Determine whether the environment value is guard-owned atomically with the refcount, or recheck the active state before taking this no-op branch.
Useful? React with 👍 / 👎.
| for key in _OFFLINE_ENV_KEYS: | ||
| saved_env[key] = os.environ.get(key) | ||
| os.environ[key] = "1" |
There was a problem hiding this comment.
When any guarded metadata or GGUF operation is in this window, these process-global environment writes are inherited by a concurrently spawned inference or training worker. The orchestrators use multiprocessing's spawn, so the child retains HF_HUB_OFFLINE=1 after this context restores the parent; because worker startup treats an existing variable as user-owned, a worker started during a DNS-live outage or transient gateway failure can remain cache-only after connectivity recovers and reject later downloads. Avoid exposing the temporary flags to child environments, or mark and clear auto-set values during worker startup.
Useful? React with 👍 / 👎.
| host = parsed.hostname or "" | ||
| try: | ||
| if host and urllib.request.proxy_bypass(host): | ||
| return None |
There was a problem hiding this comment.
When HF_ENDPOINT is an IP covered by a CIDR entry such as NO_PROXY=10.0.0.0/8, or the bypass entry includes the endpoint port such as huggingface.co:443, the Hub client's requests proxy logic bypasses the proxy but urllib.request.proxy_bypass(host) does not. This helper therefore probes through the configured proxy even though the real Hub request goes direct; if that proxy cannot reach the internal endpoint, the probe marks a reachable endpoint offline and uncached loads fail immediately. Apply the same CIDR and host-with-port matching used by the Hub client's proxy selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2249ea0f97
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| from utils.transformers_version import hf_endpoint_unreachable | ||
| _offline = hf_endpoint_unreachable(gateway_errors_offline = False) |
There was a problem hiding this comment.
When a training job is launched with only TRANSFORMERS_OFFLINE=1 set, this new endpoint probe still runs because the surrounding guard only checks for HF_HUB_OFFLINE. On a DNS-resolving but WAN-down machine that means an explicitly offline worker now emits a network HEAD request and waits for its timeout before startup; use the shared offline-env check before hf_dns_dead()/hf_endpoint_unreachable() so TRANSFORMERS_OFFLINE remains authoritative.
Useful? React with 👍 / 👎.
| def _probe_raising(self, monkeypatch, exc): | ||
| import urllib.request | ||
|
|
||
| monkeypatch.setattr(urllib.request, "urlopen", lambda *a, **k: (_ for _ in ()).throw(exc)) |
There was a problem hiding this comment.
When the test runner has HTTPS_PROXY/ALL_PROXY set, hf_endpoint_unreachable() takes its new proxy-opener path and calls opener.open instead of the urllib.request.urlopen symbol patched here. These timeout tests then make real proxy/network calls and can pass or fail based on ambient connectivity rather than the injected URLError; clear proxy env for this class or patch build_opener/hf_proxy_for_endpoint as the proxy-specific tests do.
Useful? React with 👍 / 👎.
| host = parsed.hostname or "" | ||
| try: | ||
| if host and urllib.request.proxy_bypass(host): | ||
| return None |
There was a problem hiding this comment.
When HTTPS_PROXY is set but the endpoint is bypassed with a port-scoped entry such as NO_PROXY=huggingface.co:443, this passes only the bare hostname to proxy_bypass, so the bypass is missed and the new reachability probe goes through the proxy even though the Hub client would connect directly. If that proxy is unavailable while direct egress works, Studio incorrectly forces uncached metadata/model loads into offline mode; include the endpoint port/netloc in the bypass check or delegate proxy selection to the Hub client's transport.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d5c2636ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return False | ||
| model = config.get("model_name") | ||
| try: | ||
| return bool(model) and is_local_path(model) |
There was a problem hiding this comment.
When model_name is a local LoRA directory whose adapter_config.json names a remote Hub base and no hf_dataset is configured, this returns True solely because the adapter path is local. run_training_process() consequently skips both reachability checks even though activate_transformers_for_subprocess() resolves that remote base and later training/security code accesses it. On a DNS-dead or WAN-down machine, a cached-base training job therefore runs through Hub metadata/model retry paths instead of enabling the worker's cache-only fallback; inspect the local adapter's recorded base before classifying the job as filesystem-only, as the inference worker already does.
Useful? React with 👍 / 👎.
| # DNS-probe wrap so offline loads skip 30-60s of soft-failed network | ||
| # checks before the worker starts. | ||
| with _hf_offline_if_dns_dead(): | ||
| with _hf_offline_if_unreachable_for(model_identifier): |
There was a problem hiding this comment.
When a remote load encounters slow DNS or an endpoint whose HEAD request times out but whose TCP handshake succeeds, entering this context synchronously runs hf_dns_dead() and hf_endpoint_unreachable(), including their thread joins and possible follow-up TCP timeout (roughly six seconds in the default direct-connect path). _load_model_impl() is an async function directly awaited by the route, so this blocks every unrelated API request and generation-control action on the event loop; the new validation and models handlers already avoid the same problem with asyncio.to_thread. Run this guard and ModelConfig.from_identifier() in a worker thread as well.
Useful? React with 👍 / 👎.
…off the loop for PR #7591
|
Round 12: two items, both correct, both regressions from my own round-11 change. Fixed in
if not (model and is_local_path(model)):
return False
base = _recorded_local_adapter_base(model)
return not base or is_local_path(base)
def _resolve_config():
with _hf_offline_if_unreachable_for(model_identifier):
return ModelConfig.from_identifier(...)
config = await asyncio.to_thread(_resolve_config)Tests. Added Verification. CI-equivalent invocation 795 passed, 22 deselected, 6 subtests passed. Full suite against a same-tip |
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ac914a2ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _probe_base = _recorded_local_adapter_base(_probe_model) | ||
| if "HF_HUB_OFFLINE" not in os.environ and not _hub_targets_are_local(_probe_model, _probe_base): |
There was a problem hiding this comment.
When a local full checkpoint's config.json records a remote model_name or _name_or_path, this gate examines only adapter_config.json and classifies the job as filesystem-only. Later, _resolve_base_model() deliberately resolves that recorded remote base and _activate_transformers_version() performs Hub metadata reads for it, so a WAN-down/DNS-live load can still incur the metadata retry timeouts that the worker probe is intended to avoid. Include the full-checkpoint base used for tier activation when deciding whether to skip the probe.
Useful? React with 👍 / 👎.
|
Round 13: correct, and the same class of bug as round 12. Fixed in
Rather than add a third copy of that logic, there is now one disk-only reader next to the resolver it mirrors, in def recorded_local_base(model_name) -> "tuple[str | None, bool]":Both worker gates delegate to it. It lives there because that module is import-light (stdlib plus local utils), so calling it from the gate cannot pin the default transformers into Tests. Added This also corrected one of my own round-12 tests. It asserted a baseless adapter was filesystem-only; it is not, since the resolver falls through to a Hub call. Renamed and flipped. Verification. Reverting the fix fails 5 tests, restoring them passes. Full suite against a same-tip Unrelated, but worth flagging separately since it cost me a detour: |
for more information, see https://pre-commit.ci
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Merged The Python 3.10-3.13 and Core jobs went red on three tests. Only one belongs to this PR:
monkeypatch.setattr(route, "_hf_offline_if_unreachable", contextlib.nullcontext)
monkeypatch.setattr(route, "_hf_offline_if_unreachable_for", contextlib.nullcontext)18 passed.
Worth correcting something I said last round: I reported this branch as fixing those two Vulkan failures. That was an artifact of comparing a tree that had not yet merged the Verification. Full suite on the merged tree: 21 failed, 12948 passed, 77 errors, and the failure identities are byte-identical to the same-tip |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ac9da099d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| base = cfg.get(_key) | ||
| if isinstance(base, str) and base and not _is_same_path(base, root): | ||
| return base, False | ||
| return None, adapter_cfg |
There was a problem hiding this comment.
When a local LoRA contains only adapter_model*.safetensors/.bin and follows the supported unsloth_<model>_<timestamp> naming convention, it has neither JSON file, so this returns (None, False). Both the inference and training worker gates consequently classify the job as filesystem-only and skip reachability detection, even though _resolve_base_model() later derives a remote unsloth/... base from that directory name and tier activation reads its Hub metadata. On a WAN-down/DNS-live machine, such a cached adapter therefore re-enters Hub retry paths and may select the default Transformers tier; account for the adapter-only inference here or report needs_hub=True when adapter weights exist without recorded metadata.
Useful? React with 👍 / 👎.
|
Round 15: correct, and it is the third time the gate has read less than the resolver. Fixed in
# Only reachable without a Hub call when there is no adapter_config.json; with one,
# the resolver tries get_base_model_from_lora first, which needs_hub already covers.
if not adapter_cfg and root.name.startswith("unsloth_") and _has_adapter_weights(root):
parts = root.name.split("_")
if len(parts) >= 2:
return "unsloth/" + "_".join(parts[1:-1]), FalseThe more useful part is why the guard I added last round did not catch this: Separately, verifying this turned up a real defect in two of this PR's own test files. sys.modules.setdefault("structlog", _types.ModuleType("structlog"))That puts an empty module into Verification. Full suite: 21 failed, 12949 passed, 77 errors, failure identities byte-identical to the same-tip |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a77a9f8e5c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # be turned cache-only here. Matches the worker's call. | ||
| unreachable = hf_endpoint_unreachable( | ||
| timeout, | ||
| gateway_errors_offline = False, |
There was a problem hiding this comment.
When the Hub or a configured mirror is returning 502/503/504, this shared verdict reports the endpoint as reachable because gateway_errors_offline is forced to False. The per-request _hf_offline_if_unreachable() guard reaches this helper via _hf_unreachable(), so cached /load, /validate, and metadata preflights do not enter force_hf_offline() during a Hub outage and can still take the Hub retry/failure path instead of staying cache-only; reserve the fail-open gateway behavior for worker lifetime flags or pass the default for scoped guards.
Useful? React with 👍 / 👎.
Git merged all three files without a conflict, but the result was wrong in each case. The tests only started failing once the merged tree was run. routes/inference.py duplicated the GGUF load block: this branch had moved the gguf_load_in_flight marker and the _hub_download_blocks_gguf_load guard under "if config.is_gguf and config.gguf_hf_repo", and the conflict resolution re-added main's copy at the old position, so both ran. Dropped main's copy; the earlier placement is the deliberate one, so a 409 from the hub guard cannot tear down a resident Images or Video pipeline. test_gpu_selection.py still called _hf_offline_if_dns_dead, which main renamed to _hf_offline_if_unreachable_for (#7591). Disjoint edits, so no conflict, but four route-error tests referenced a function that no longer exists. test_gguf_load_cache_reuse.py anchored its ordering assertion with rindex over "if config.is_gguf:", taking the last one before the load marker. That only held while _resolve_inherited_extra_args sat above every such line; main has since hoisted it above the gpu_ids preflight, so the anchor landed between the call and the marker and the assertion compared against an unrelated later call site. The ordering it checks is a property of _load_model_impl as a whole, so it now anchors on the function. The ordering itself is intact: _resolve_inherited_extra_args, then the gguf_load_in_flight marker, then the hub guard, then the chat handoff, then unload_model.
Rebase onto main (resolves the merge conflicts from the four-month gap) and extend the mirror support into the hub module Daniel flagged: Backend - utils/hf_endpoint.py: get_hf_endpoint() now delegates to utils.utils.hf_endpoint_url() (landed via unslothai#7591) so HF_ENDPOINT has a single parsing implementation; keeps the HF_DATASETS_SERVER split and its once-only mirror warning. Exposed via /api/health. - Replaced the remaining hardcoded huggingface.co URLs in orchestrator.py, model_config.py and data_recipe/huggingface.py. - transformers_version.py: kept upstream's URL routing; added graded logging (404 stays debug, 5xx/URLError warn with a mirror hint) so a broken mirror no longer hides behind a silent debug line. Frontend - lib/hf-endpoint.ts + config/env.ts: endpoint values flow from /api/health into the platform store (hf_endpoint is in the unauthenticated health block, so it works pre-login). - Ported the search hooks onto the new hub layout: use-hub-model-search (10 listModels/cachedModelInfo call sites), use-hub-dataset-search, use-hf-dataset-splits. - Hub lib de-hardcoded: hf-readme, hf-owner-avatar, dataset-size (datasets-server via HF_DATASETS_SERVER), and network.ts — the offline backoff now keys on the configured endpoint's origin instead of huggingface.co, matching where requests actually go. - Hub catalog external links and the seed-config default endpoint follow the configured mirror; dropped the pinned endpoint from the instruction-from-answer template so it stops overriding the default.
3 participants
The problem
A user reported that after downloading a model, turning off the internet and reloading it, Studio tries to fetch
config.jsonfrom Hugging Face and the model never becomes usable.It reproduces. Loading an already-downloaded GGUF with no connectivity took 11.4 minutes, of which 681 seconds were failed hub calls and 5.1 seconds were real work:
The model is not broken. Inference answers in 202 ms once loaded. The load just looks like a hang.
Root cause
Two separate problems, both in the offline guard.
1. We only checked DNS.
_hf_offline_if_dns_deaddecided we were offline only ifhuggingface.cofailed to resolve. Plenty of genuinely offline setups still resolve names: WAN down behind a live router, captive portal, VPN split-DNS, corporate resolver, or just a stale OS DNS cache right after toggling wifi off. In those the guard never fired.The three detectors, measured with the endpoint blackholed:
The correct probe already existed and was already shipping in the export path. It just was not used anywhere else.
2. Setting the env vars mid-process does nothing. Even once the guard fired,
huggingface_hubandtransformersread their offline constants at import time, and hub sessions cache a non-offline adapter.os.environ["HF_HUB_OFFLINE"] = "1"after startup left every call retrying as before. This is why the first version of this fix only got 686s down to 385s.The fix
utils/utils.py:hf_unreachable(), the bounded proxy-aware probe memoised for 60s, andforce_hf_offline(), which flips the in-process constants and rebuilds hub sessions, restoring everything on exit.llama_cpp.py: the guard escalates DNS -> reachability and forces offline in-process. Renamed_hf_offline_if_dns_deadto_hf_offline_if_unreachablesince it is no longer DNS-only./models/config,/models/check-vision,/picker/chat-template, and the per-request vision probe in_target_is_vision.huggingface.coand ignoredHF_ENDPOINT, so mirror users got the wrong answer even online.Results
Cached GGUF repo, endpoint blackholed:
POST /inference/loadGET /models/configGET /models/check-visionGET /picker/chat-templateCompatibility
Online behaviour is unchanged. The probe only runs when the offline env vars are not already set, and when the endpoint is reachable the guard is a no-op, so the online path is byte-identical to today. Verified on the patched build with real internet:
unsloth/SmolLM2-135M-Instruct-GGUFresolves, downloads, loads and chats in 4sThree further safeguards:
hf_endpoint_unreachabletreats an HTTP or TLS response as reachable, so only a genuine connection failure counts as offline.hf_unreachable()fails open: if the probe itself errors, we report reachable and let the load decide.UNSLOTH_OFFLINE_PROBE=0restores the old DNS-only behaviour, matching the existing opt-out in the export path.No new hardware or backend path is introduced. Nothing touches GPU selection, llama.cpp arguments, model formats or training kernels.
Tests
Full studio backend suite, before and after, on the same machine:
The failure lists are byte-identical (
diffreturns nothing). Those 31 are pre-existing environment failures on this box: no CUDA, flash-attn, cache-settings. The +6 are new tests here.Added to
test_offline_gguf_cache_fallback.py:UNSLOTH_OFFLINE_PROBE=0-> DNS-only behaviour preservedExisting guard tests now pin reachability explicitly so no test can reach the network.
Not covered here
/models/check-visionreturnsis_vision: falsefor a GGUF repo that ships anmmproj. That is pre-existing and unrelated to connectivity (it reproduces online too):is_vision_modelonly does the mmproj check for local paths, while the load path resolves it correctly viaModelConfig.from_identifier. This PR makes that route fast but deliberately leaves its semantics alone. Happy to fix separately.