-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Studio: Auto download transport, Xet memory caps, and stall detection on the hub path #7742
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.
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
Changes from 12 commits
077f0af
5f78e7d
cab7c30
868e0dd
1be7e51
1a0654e
9fa856e
74f85f2
d1db7d4
c4d6fd1
cbe0ec7
f465e47
db19179
2d7b96b
fc8eae8
f79b0ce
ec6c6f2
013e8b6
7dfbc55
c068acd
49fe5c8
3c8b1ff
dd043e8
6ceeab2
c5c80a3
5f3e783
47c27b0
46fb2bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,49 @@ def resolve_effective_use_xet(use_xet: bool) -> bool: | |
| return False | ||
|
|
||
|
|
||
| def resolve_requested_use_xet(transport_mode: Optional[str], use_xet: bool) -> tuple[bool, str]: | ||
| """Turn a download request's transport preference into ``(use_xet, reason)``. | ||
|
|
||
| ``transport_mode`` is the current field ("auto" / "xet" / "http"); ``use_xet`` is the older | ||
| boolean, still honoured so an older frontend (or a scripted API caller) keeps working. An | ||
| explicit "xet" is respected even on a machine the health check dislikes -- the user asked -- but | ||
| it still gets the memory caps and the stall fallback. | ||
| """ | ||
| mode = (transport_mode or "").strip().lower() | ||
| if mode == download_registry.TRANSPORT_HTTP: | ||
| return (False, "HTTP (requested)") | ||
| if mode == download_registry.TRANSPORT_XET: | ||
| return (resolve_effective_use_xet(True), "Xet (requested)") | ||
| if mode == download_registry.TRANSPORT_AUTO: | ||
| return resolve_auto_use_xet() | ||
| resolved = resolve_effective_use_xet(use_xet) | ||
| return (resolved, "Xet" if resolved else "HTTP") | ||
|
|
||
|
|
||
| def resolve_auto_use_xet() -> tuple[bool, str]: | ||
| """Pick a transport for a download the user left on "Auto". Returns ``(use_xet, reason)``. | ||
|
|
||
| Server-side on purpose: only the backend can see this machine's RAM, its hf_xet build, and | ||
| whether Xet has been failing here, and the browser must not have to guess any of it. | ||
|
|
||
| Probing IS allowed here (unlike on the per-download path) because Auto resolution happens once | ||
| per download request and the answer is cached for the whole machine -- a few hundred ms buys a | ||
| verdict that saves every subsequent download a stalled attempt. | ||
| """ | ||
| if not resolve_effective_use_xet(True): | ||
| return (False, "hf_xet is not installed") | ||
| try: | ||
| from utils.hf_xet_fallback import xet_health | ||
| health = xet_health() | ||
| except Exception as exc: # noqa: BLE001 - never let a health probe block a download | ||
| logger.debug("Xet health probe failed, defaulting to Xet: %s", exc) | ||
| return (True, "Xet (health check unavailable)") | ||
| if health is None: | ||
| # Older unsloth_zoo without the health module: no opinion, keep the existing default. | ||
| return (True, "Xet") | ||
| return (bool(health.use_xet), str(health.reason)) | ||
|
|
||
|
|
||
| def resolve_transport(use_xet: bool) -> str: | ||
| transport = download_registry.TRANSPORT_XET if use_xet else download_registry.TRANSPORT_HTTP | ||
| unavailable_reason = download_registry.download_transport_unavailable_reason(transport) | ||
|
|
@@ -82,6 +125,36 @@ def spawn_worker( | |
| env["HF_HUB_DISABLE_PROGRESS_BARS"] = "1" | ||
| env["HF_HUB_DISABLE_TELEMETRY"] = "1" | ||
| env["HF_HUB_DISABLE_XET"] = "0" if use_xet else "1" | ||
| if use_xet: | ||
| # hf_xet sizes its reconstruction buffers from constants (up to 8GB stock, 64GB under | ||
| # high-performance mode), not from the machine. Cap them from this host's RAM and cores | ||
| # BEFORE the worker starts: hf_xet reads its config natively at import, so setting these | ||
| # inside the worker would be too late. Anything already in `env` was set deliberately by | ||
| # the caller and is left alone. | ||
| from utils.hf_xet_fallback import xet_env_overrides | ||
|
|
||
| allow_high_perf = os.environ.get( | ||
| "UNSLOTH_XET_ALLOW_HIGH_PERFORMANCE", "" | ||
| ).strip().lower() in ( | ||
| "1", | ||
| "true", | ||
| "yes", | ||
| "on", | ||
| ) | ||
| if not allow_high_perf: | ||
| # Unconditional, and deliberately not routed through xet_env_overrides(): an older | ||
| # unsloth_zoo has no tuning module, so the overrides come back empty -- and that same | ||
| # older zoo is the one that sets HF_XET_HIGH_PERFORMANCE=1 at import. `env` is seeded | ||
| # from the parent environment, so the inherited "1" would arrive here and raise the | ||
| # buffer ceiling to 64GB, voiding every cap below (xet-core applies the | ||
| # high-performance preset AFTER reading the environment). Overwrite, never setdefault. | ||
| for key in ("HF_XET_HIGH_PERFORMANCE", "HF_XET_HP"): | ||
| env[key] = "0" | ||
| for key, value in xet_env_overrides().items(): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On a cold backend when the background warm-up is disabled, unfinished, or skipped on a CPU-only host, an explicit or legacy Xet request reaches this call without the Auto health probe having loaded Zoo first. Both async download handlers invoke Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| if key in ("HF_XET_HIGH_PERFORMANCE", "HF_XET_HP") and not allow_high_perf: | ||
| env[key] = value | ||
| else: | ||
| env.setdefault(key, value) | ||
| # No token in Unsloth settings: fall back to the backend's own HF_TOKEN so | ||
| # private repos stay downloadable (needed while inkling repos are private). | ||
| # Not for a repo an API caller named: that would lend them the owner's identity. | ||
|
|
@@ -554,6 +627,101 @@ def kill_and_reap_process( | |
| pass | ||
|
|
||
|
|
||
| def _record_xet_failure(reason: str, logger) -> None: | ||
| """Tell the health tracker a Xet transfer failed here; best-effort, never fatal to a download.""" | ||
| try: | ||
| from utils.hf_xet_fallback import record_xet_outcome | ||
| record_xet_outcome(False, reason) | ||
| except Exception as exc: # noqa: BLE001 | ||
| logger.debug("could not record Xet outcome: %s", exc) | ||
|
|
||
|
|
||
| def _repo_bytes_on_disk(repo_type, repo_id: str, cache_dir) -> "Optional[int]": | ||
| """Bytes present for this repo, or None when unmeasurable. | ||
|
|
||
| Used only to tell an actual Xet transfer from a job that found everything already cached: the | ||
| worker reports nothing but an exit code, and the .transport marker is written before the | ||
| transfer starts, so there is no other signal. | ||
| """ | ||
| try: | ||
| from utils.hf_xet_fallback import get_hf_download_state | ||
| state = get_hf_download_state([repo_id], repo_type = repo_type, cache_dir = cache_dir) | ||
| except Exception: # noqa: BLE001 - a missing measurement must never fail a download | ||
| return None | ||
| return None if state is None else int(state[0]) | ||
|
|
||
|
|
||
| def _record_xet_success(logger) -> None: | ||
| """Tell the health tracker a Xet transfer completed here, which resets the failure streak.""" | ||
| try: | ||
| from utils.hf_xet_fallback import record_xet_outcome | ||
| record_xet_outcome(True, "Xet download completed") | ||
| except Exception as exc: # noqa: BLE001 | ||
| logger.debug("could not record Xet outcome: %s", exc) | ||
|
|
||
|
|
||
| def _start_stall_watchdog( | ||
| registry: download_registry.DownloadRegistry, | ||
| key: str, | ||
| proc: subprocess.Popen, | ||
| *, | ||
| repo_type: RepoType, | ||
| repo_id: str, | ||
| label: str, | ||
| log_prefix: str, | ||
| logger, | ||
| on_stall: Callable[[str], None], | ||
| ): | ||
| """Kill *proc* if its download stops making byte-level progress. Returns a stop event, or | ||
| ``None`` when no watchdog could be started. | ||
|
|
||
| SIGKILL rather than a polite signal: the worker traps SIGTERM and exits 130 ("cancelled"), which | ||
| would be recorded as a user cancel and skip the HTTP retry. An untrapped kill lands as "error", | ||
| which is the state that triggers the retry. The worker's writer is sequential and resumable, so | ||
| the partial it leaves behind is not lost work. | ||
| """ | ||
| try: | ||
| from utils.hf_xet_fallback import start_watchdog | ||
| except Exception as exc: # noqa: BLE001 - degraded unsloth_zoo: keep the old behaviour | ||
| logger.debug("%s stall watchdog unavailable for %s: %s", log_prefix, label, exc) | ||
| return None | ||
|
|
||
| metadata = registry.get_job_metadata(key) | ||
| cache_dir = getattr(metadata, "hub_cache", None) if metadata is not None else None | ||
|
|
||
| def _on_stall(message: str) -> None: | ||
| logger.warning( | ||
| "%s %s for %s; killing the worker to retry over HTTP", log_prefix, message, label | ||
| ) | ||
| on_stall(message) | ||
| try: | ||
| # Kill only -- the _watch thread is already blocked reaping this process, and a second | ||
| # wait() here would just race it for the exit status. | ||
| proc.kill() | ||
| except ProcessLookupError: | ||
| pass # already exited between the stall verdict and the kill | ||
| except Exception: | ||
| logger.exception("%s failed to kill stalled worker for %s", log_prefix, label) | ||
|
|
||
| try: | ||
| return start_watchdog( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new hub watchdog call relies on the shared Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing an explicit value here would break the intended behaviour: the shared layer resolves |
||
| repo_ids = [repo_id], | ||
| repo_type = repo_type, | ||
| cache_dir = cache_dir, | ||
| on_stall = _on_stall, | ||
| child_pid = proc.pid, | ||
| # Scope the measurement to partials this worker actually holds open. Without it the | ||
| # shared helper stays repo-wide and child_pid does nothing, so two same-transport GGUF | ||
| # variants of one repo (which the registry deliberately allows to run concurrently) | ||
| # reset each other's stall timer and a hung variant never falls back. | ||
| watch_new_partials_only = True, | ||
| xet_disabled = False, | ||
| ) | ||
| except Exception as exc: # noqa: BLE001 | ||
| logger.debug("%s could not start stall watchdog for %s: %s", log_prefix, label, exc) | ||
| return None | ||
|
|
||
|
|
||
| def register_worker( | ||
| registry: download_registry.DownloadRegistry, | ||
| key: str, | ||
|
|
@@ -574,8 +742,17 @@ def register_worker( | |
| return False | ||
|
|
||
| worker_token = hf_token | ||
| # getattr, not a direct call: test doubles and older registries do not all implement this. | ||
| _get_metadata = getattr(registry, "get_job_metadata", None) | ||
| _metadata = _get_metadata(key) if callable(_get_metadata) else None | ||
| _cache_dir = getattr(_metadata, "hub_cache", None) if _metadata is not None else None | ||
| # Sampled before the worker can write anything, so "did this job actually move bytes over Xet" | ||
| # is answerable when it exits. | ||
| _bytes_before = _repo_bytes_on_disk(repo_type, repo_id, _cache_dir) | ||
|
|
||
| def _watch() -> None: | ||
| stalled: list[str] = [] | ||
| watchdog_stop = None | ||
| try: | ||
| can_retry_http = ( | ||
| transport == download_registry.TRANSPORT_XET | ||
|
|
@@ -584,6 +761,23 @@ def _watch() -> None: | |
| ) | ||
| is None | ||
| ) | ||
| # Until now this path had NO stall detection at all: it relied on the worker's own exit | ||
| # code, and a Xet transfer that hangs with no progress and no error never produces one. | ||
| # That is the common failure the model-hub page shows as a frozen progress bar. Watch | ||
| # the cache for byte-level progress and kill a hung worker so the HTTP retry below can | ||
| # take over; the SIGKILL surfaces as "error", which is exactly what triggers it. | ||
| if can_retry_http: | ||
| watchdog_stop = _start_stall_watchdog( | ||
| registry, | ||
| key, | ||
| proc, | ||
| repo_type = repo_type, | ||
| repo_id = repo_id, | ||
| label = label, | ||
| log_prefix = log_prefix, | ||
| logger = logger, | ||
| on_stall = stalled.append, | ||
| ) | ||
| state = finalize_worker_exit( | ||
| registry, | ||
| key, | ||
|
|
@@ -598,6 +792,30 @@ def _watch() -> None: | |
| cancel_marker_transport = cancel_marker_transport, | ||
| defer_error = can_retry_http, | ||
| ) | ||
| if watchdog_stop is not None: | ||
| # Stop measuring the moment the worker is reaped: post-download work (symlinking, | ||
| # verification) makes no byte-level progress and must not read as a stall. | ||
| watchdog_stop.set() | ||
|
Comment on lines
+835
to
+838
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the initial metadata request fails but the Xet transfer itself succeeds, the worker remains alive after its last blob while Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The structural read is right and worth stating plainly: the stop event does sit after finalize_worker_exit, so the watchdog is armed across the whole post-byte phase. The deadline in that window is not 30s though. After the last blob is renamed the child holds no .incomplete open, so with watch_new_partials_only the measured size drops to zero, which resets the clock at the rename and routes the phase to the not-has_incomplete-and-seen_bytes_ever branch. That branch is armed with connect_timeout = 600.0 from _start_stall_watchdog, not the 30s stall timeout, and the 30s one needs an open partial the child is holding. Against that, your 41s is the right worst case and I confirmed the constants: _METADATA_REQUEST_TIMEOUT 10.0 plus _METADATA_RETRY_DELAY 1.0 plus _METADATA_RETRY_TIMEOUT 30.0. Everything after it is disk only, and _verify_completed_download stats each manifest file against its declared size rather than hashing, so it does not grow with model size. 41s against 600s is not reachable. On the 2026.8.1 floor there is nothing to trip either: the kwarg is filtered out and that release resets its timer whenever no partial exists, which is exactly this phase. |
||
| if stalled: | ||
| # A machine whose Xet transfers hang is one that should stop starting on Xet. | ||
| _record_xet_failure(stalled[0], logger) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a Xet worker exits with a retryable CAS/network error before the stall watchdog fires, Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The recording is deliberately scoped to stalls. A fast CAS or 5xx error costs one quick HTTP retry, whereas a hang costs the full stall timeout, and counting transient blips would demote machines on failures HTTP would have hit too. That noise is what the two-strike threshold exists to absorb. |
||
| elif transport == download_registry.TRANSPORT_XET and state == "complete": | ||
| # Clear the streak, so "two failures in a row" means in a row. Without this a | ||
| # stall today and another next week are counted as consecutive despite every | ||
| # download in between succeeding, pinning Auto to HTTP for no reason. | ||
| # | ||
| # Only a job that actually moved bytes says anything about Xet's health, though: a | ||
| # fully cached repo (the UI's re-download action on an up-to-date model) exits 0 | ||
| # without touching the network, and clearing a correctly earned demotion on that | ||
| # would put a bad machine back on Xet. Unmeasurable means do not clear -- a missed | ||
| # clear costs one extra streak entry, a wrong clear undoes the demotion. | ||
| bytes_after = _repo_bytes_on_disk(repo_type, repo_id, _cache_dir) | ||
| if ( | ||
| _bytes_before is not None | ||
| and bytes_after is not None | ||
| and bytes_after > _bytes_before | ||
| ): | ||
| _record_xet_success(logger) | ||
| # XET-to-HTTP recovery: when a non-cancelled XET worker fails and | ||
| # HTTP is available, attempt one automatic retry over HTTP. The | ||
| # transport check is the recursion guard: an HTTP worker that errors | ||
|
|
@@ -615,6 +833,8 @@ def _watch() -> None: | |
| watch_name = watch_name, | ||
| ) | ||
| except Exception: | ||
| if watchdog_stop is not None: | ||
| watchdog_stop.set() | ||
| # finalize_worker_exit is the only thing that clears running/cancelling; | ||
| # if it raises, force a terminal state so claim() isn't blocked until restart. | ||
| logger.exception("download watcher crashed for %s", key) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clients that use the new backend
transport_mode: "auto"field directly, this call reads the health verdict without opting into a probe, even though the download-start path is the moment intended to pay that cost. On a host withhf_xetinstalled, no cached health verdict, and an unreachable CAS endpoint, Auto will still choose the optimistic Xet path and only fall back after the stall watchdog fires; passprobe=Truehere so backend-resolved Auto avoids the failed Xet attempt up front.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already doing what you are asking for. xet_health in the companion zoo is declared
def xet_health(*, force: bool = False, probe: bool = True), so the bare call in resolve_auto_use_xet does run the reachability probe; it is the capabilities endpoint that explicitly passes probe=False, because the UI polls that one on render. The split is deliberate and the docstring right above the call says so.So on a host with hf_xet installed, no cached verdict and an unreachable CAS, backend Auto resolves to HTTP up front rather than paying a stalled Xet attempt. Both download callers already run the resolution through asyncio.to_thread, with the 3s probe budget, so it never sits on the event loop even when DNS is blackholed.