Derive download buffer defaults from usable, cgroup-aware system memory - #943
Conversation
The three HF_XET_RECONSTRUCTION_DOWNLOAD_BUFFER_* defaults were compile-time constants (2GB/512MB/8GB): too small for fast large-memory hosts (2.7x throughput left on a 20 Gbit/s machine) and too large for small containers (OOM kills at 1Gi). Derive them at startup from min(host RAM, effective cgroup limit) with fraction-plus-clamp formulas anchored to reproduce the old defaults at 32GB and the old high-performance constants at >=256GB. The cgroup probe walks the paths in /proc/self/cgroup (v1 and v2, nested paths, namespaced roots) instead of the controller root, which is what sysinfo reads and what breaks under Slurm/systemd nesting. Also in this change: - HF_XET_HP buffer values are memory-derived too (no more 16GB base buffer on an 8GB laptop), and env overrides now beat the HP preset instead of being silently stomped (apply_env_overrides now falls back to the current field value rather than re-evaluating the default). - Setting DOWNLOAD_BUFFER_SIZE above the limit no longer panics at context construction; the limit is normalized up with a warning. - Kill switch: HF_XET_MEMORY_DERIVED_DOWNLOAD_BUFFERS=0 restores the static defaults. Per-knob env vars still override as before. Fixes #927 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGuU1K7rpjFfBGrA3SvCxL
…ory probe Replace the hand-rolled /proc/self/cgroup walk with sysinfo 0.39's Process::cgroup_limits, which resolves the process's cgroup path and takes the tightest memory limit across ancestor cgroups (v1 and v2) — the container-aware behavior this change needs, now maintained upstream instead of in this repo. The 10 in-repo cgroup fixture tests go with the parser; sysinfo carries equivalent fixtures upstream. - sysinfo 0.38.4 -> 0.39.6 (workspace pin 0.39; all four lockfiles) - Rust toolchain 1.94.1 -> 1.95.0 in all CI workflow pins (sysinfo 0.39 MSRV; the wasm sub-workspace stays on its pinned nightly) - When no cgroup limit binds, the reported limit equals the host total and is filtered out so cgroup_limit = None keeps meaning "no limit set" Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGuU1K7rpjFfBGrA3SvCxL
|
let's make the rust version used in CI consistent (all the same) |
Sounds good, will make related PRs in the other repos that directly depend on xet-core. |
…feedback) sysinfo moves to the cfg(not(target_family = "wasm")) dependency section of xet_runtime, so wasm builds no longer pull it in at all; its three consumers (system_monitor, logging::init, the system_memory probe) were already wasm-gated. Side benefit: wasm consumers are no longer subject to sysinfo's 1.95 MSRV. The api_changes note is removed per review; the PR description carries the downstream notes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGuU1K7rpjFfBGrA3SvCxL
|
@assafvayner ready for re-review. Also, cargo audit is fixed in #950. |
- Raise derivation floors to the 1 GiB values (64 MB / 16 MB / 264 MB), dropping the <=512 MB tier; the smallest configuration now holds a maximum-size (64 MiB) term once one file download is active. - Define the derivation ceilings from STATIC_HP_DEFAULTS instead of repeating the numbers. - Hoist the LazyLock caches to module-level statics (USABLE_MEMORY, DERIVED_DEFAULTS) and replace the derived tuple with a named struct; the public accessors remain functions because they consult the kill-switch env var on every call. - Normalize the reconstruction group in XetConfig::new() as well, so the caller-visible config is coherent; XetContext::new keeps the choke-point normalize for configs mutated after construction (e.g. with_config). - Use EnvVarGuard::unset in the test helper instead of raw remove_var. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGuU1K7rpjFfBGrA3SvCxL
assafvayner
left a comment
There was a problem hiding this comment.
Approving conditional on the docs+log fix
…docstrings - The derived-defaults info! line was emitted inside the DERIVED_DEFAULTS LazyLock initializer, which hf_xet first forces via the XetConfig::new() call inside xet_pkg::init_logging — before logging::init installs the tracing subscriber. The event was dropped and, the initializer running only once, never fired again. Move the logging into log_derived_defaults(), called by logging::init after the subscriber is installed. Verified against the real init ordering. - Update the reconstruction.rs docstring clamp bounds to the current floors (64MB / 16MB / 264MB). - Rename derive() to derive_download_buffer_defaults(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGuU1K7rpjFfBGrA3SvCxL
|
Manual container verification of the probe (7bd1427, Docker Desktop Linux VM with 8.3 GB, throwaway binary reproducing the hf_xet init ordering —
In every case One edge found while testing: a process placed in a cgroup whose memory controller is not enabled at the leaf (no This verification was AI-assisted (run and reviewed by @rajatarya). |
…D_BUFFERS Per review: the switch is now a disable flag — set =1 to restore the static defaults; unset (the default) keeps memory-derived values. Updated the parse logic, docstrings, log message, and tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGuU1K7rpjFfBGrA3SvCxL
…udit) Transitive dependency (via rand 0.10); 0.10.0 and 0.10.1 were yanked upstream, which trips `cargo audit -D warnings` on every branch, including main. Bumped in all four lockfiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGuU1K7rpjFfBGrA3SvCxL
Memory-derived download buffer defaults
Fixes #927. Related: huggingface_hub#3300.
Problem
The three
HF_XET_RECONSTRUCTION_DOWNLOAD_BUFFER_*defaults are compile-time constants (2 GB / 512 MB / 8 GB). Issue #927 measured the cost on both tails: a 20 Gbit/s, 2 TB host leaves 2.7x throughput on the table (6709 → 18114 Mbit/s from raising only these three knobs), while a 1 Gi Kubernetes container gets OOM-killed (huggingface_hub#3300). A default that is simultaneously too small for a 2 TB host and too large for a 1 Gi container should be a function of available memory, not a constant.Design overview
Three cooperating changes, all inside
xet_runtime:utils/system_memory.rs, new): usable memory = min(host total, effective cgroup limit), cgroup-aware in the way the issue prescribes — resolving the path from/proc/self/cgrouprather than reading the controller root, via sysinfo 0.39'sProcess::cgroup_limits(). This requires sysinfo 0.38.4 → 0.39.6 and a Rust toolchain bump 1.94.1 → 1.95.0 (sysinfo 0.39's MSRV), both part of this PR.clamp(fraction × usable, floor, ceiling), anchored so a 32 GB machine reproduces today's defaults and a ≥256 GB machine reproduces today's high-performance preset. Explicit env vars override, exactly as before.SIZE > LIMITno longer panics (normalized with a warning); the HP preset becomes memory-aware and no longer silently stomps explicit env vars.The memory probe
flowchart TD A(["USABLE_MEMORY probe"]) --> B{"LazyLock cached?"} B -->|yes| Z(["return cached UsableMemory"]) B -->|no| C["host_total via sysinfo<br/>RefreshKind memory only"] C --> D["refresh own PID entry then<br/>Process cgroup_limits<br/>from sysinfo 0.39"] D --> E{"limits returned?<br/>Linux only"} E -->|"no: macOS via Mach sysctl<br/>Windows via GlobalMemoryStatusEx<br/>wasm stub - no info"| N["cgroup_limit = None"] E -->|yes| F["sysinfo resolves the path in<br/>/proc/self/cgroup and takes the<br/>min limit across ancestor cgroups<br/>v2 memory.max or v1 limit_in_bytes"] F --> G{"limit below host total?"} G -->|"no - unlimited"| N G -->|yes| H["cgroup_limit = limit"] N --> L H --> L["usable = min of host_total<br/>and cgroup_limit"] L --> M["cache UsableMemory struct<br/>host_total / cgroup_limit / usable"] M --> ZWhy sysinfo 0.39: the previously pinned sysinfo 0.38.4 only offered
System::cgroup_limits(), which reads controller-root paths (/sys/fs/cgroup/memory.max) — fine under Docker/k8s cgroup namespaces but blind to nested cgroups (systemd slices, Slurm steps), the exact failure modes the issue reporter hit downstream. sysinfo 0.39.0 addedProcess::cgroup_limits(), which resolves the process's own cgroup path from/proc/<pid>/cgroupand takes the tightest bound across ancestor cgroups, for both cgroup v1 and v2, treatingmax/PAGE_COUNTER_MAX as unbounded. Depending on the maintained implementation beats maintaining a hand-rolled/procparser here; sysinfo carries its own fixture tests for the walk. sysinfo still supplies the host total; the probe adds no new dependency.The probe is
LazyLock-cached (theconfig_group!macro evaluates default expressions on everyXetConfig::new(), which runs per session, in the legacy runtime, twice in git_xet, and in xtool).Process::cgroup_limitsreturnsNoneoff-Linux by contract, so the probe needs no OS-specific gating of its own; wasm gets a stub returning no memory info, which falls back to today's static defaults. When no cgroup limit binds, the reported limit equals the host total and is filtered out, keepingcgroup_limit = Nonemeaning "no limit set" in logs.Platform behavior
The probe has two independent inputs — host total and container limit — and each platform fills in what it can.
usable = min(present values); if neither is available, derivation is skipped entirely and today's static defaults apply (fail-safe, never worse than current behavior)./proc/meminfo)Process::cgroup_limits(sysinfo 0.39): cgroup v2 + v1, nested paths, namespaced rootssysctl hw.memsize) — physical RAMGlobalMemoryStatusEx) — physical RAMNoneNoneNoneThe derivation formula itself is platform-independent and unit-tested everywhere. cgroup resolution is delegated to sysinfo, which is tested upstream (including nested-ancestor fixtures); this repo carries no OS-specific probe code beyond the wasm stub.
The derivation
For standard mode:
limit = clamp(u/4, 264 MB, 64 GB),size = clamp(u/16, 64 MB, 16 GB),perfile = clamp(u/64, 16 MB, 2 GB), each then rounded down to a multiple of 8 MB (decimal units throughout, matchingByteSize). The floors are the 1 GiB derivation values; the ceilings are the historical HP constants. High-performance mode uses 2x-aggressive fractions with the same floors/ceilings:u/2,u/8,u/32.Properties, verified analytically at every region boundary for both fraction sets:
size + max_concurrent_file_downloads(8) × perfile ≤ limitholds everywhere (mid-range: 3u/16 ≤ u/4; floors: 64+128=192 ≤ 264 MB; ceilings: 16+16=32 ≤ 64 GB; HP: 3u/8 ≤ u/2). The three numbers describe one allocation, as the issue requested.Floors note: per review, the floors are the 1 GiB derivation values — environments below ~1 GiB get the same triple rather than a smaller dedicated tier. At the floors a single maximum-size term (one unpacked xorb block, ≤ 64 MiB) fits the budget once one file download is active (64 + 16 = 80 MB); as a backstop,
acquire_manyclamps an oversized single term to total permits, so it proceeds serially rather than deadlocking.Startup flow — how system info reaches the consumers
sequenceDiagram autonumber participant PY as Python hf_xet participant XS as PyXetSession participant CFG as XetConfig participant MEM as system_memory probe participant CTX as XetContext participant COM as XetCommon participant SEM as AdjustableSemaphore participant FR as FileReconstructor PY->>XS: XetSession constructor XS->>CFG: XetConfig::new CFG->>CFG: Default::default builds groups CFG->>MEM: USABLE_MEMORY (first read) Note over MEM: cgroup walk then sysinfo host total<br/>cached in LazyLock for process lifetime MEM-->>CFG: usable bytes Note over CFG: derived defaults<br/>size = clamp of usable/16<br/>perfile = clamp of usable/64<br/>limit = clamp of usable/4 CFG->>CFG: with_high_performance if HP flag<br/>memory-aware HP fractions CFG->>CFG: with_env_overrides<br/>explicit env vars always win CFG-->>XS: XetConfig XS->>CTX: XetContext::with_config CTX->>CTX: normalize reconstruction group<br/>ensure limit >= size then freeze as Arc CTX->>COM: XetCommon::new COM->>SEM: new with initial size<br/>floor size and ceiling limit Note over SEM: one permit equals one byte<br/>caps in-flight unwritten bytes XS-->>PY: session ready PY->>FR: download file (later) FR->>SEM: grow target to min of<br/>size plus n_active times perfile<br/>and limit SEM-->>FR: seed permit for growth FR->>SEM: acquire_many term_size per term Note over FR,SEM: permits released only after<br/>bytes are written to diskThe same
XetConfig::new()→XetContext::new()funnel serves every entry point (PythonXetSession, the legacydownload_filesruntime still used by huggingface_hub, git_xet's LFS agent, xtool), so all of them inherit derived defaults with no per-caller changes.XetContext::newis the single choke point where the normalized config is frozen into anArc— guaranteeing the semaphore bounds (built once inXetCommon::new) and the per-download #666 target formula (recomputed inFileReconstructor) always read identical values.Ordering and preset fixes
XetConfig::new()becomes defaults → HP preset → env overrides (today env runs before HP, soHF_XET_HPsilently discards explicitHF_XET_RECONSTRUCTION_*settings — the issue's benchmark matrix would literally have been impossible to run as intended with both set). Enabling this requiresapply_env_overridesto fall back to the current field value rather than re-evaluating the macro default — a one-line macro change (let default_value = self.$name.clone()), audited safe: all existing callers outsidenew()are tests invoking it immediately after construction, and it halves default-expression evaluations as a bonus.with_high_performance()keeps its static fetch-size and concurrency literals (metadata-scale, not RAM-scale) but takes its three buffer values from the HP derivation — soHF_XET_HPon an 8 GB laptop now yields 1 GB / 248 MB / 4 GB instead of a physically impossible 16 GB base buffer.Panic fix:
AdjustableSemaphore::newrelease-assertsmin ≤ max, soHF_XET_RECONSTRUCTION_DOWNLOAD_BUFFER_SIZE=16gbalone crashes today (default limit 8 GB). Normalization (if limit < size { warn!; limit = size }) runs once inXetContext::new;XetCommon::newadditionally applieslimit.max(base)locally since it ispuband must never panic. Derived values can never trigger it by construction.Small containers and large machines
min_prefetch_buffer,min/max_reconstruction_fetch_size) are deliberately not derived: traced to be metadata-scale — they size reconstruction-term prefetch (term descriptions, URLs), not data buffers; actual data bytes acquire a buffer permit before the download task spawns. The issue's own benchmark confirms: those knobs moved throughput ≤2%, within drift.Escape hatches and observability
HF_XET_RECONSTRUCTION_DOWNLOAD_BUFFER_*env var still overrides its derived default (macro semantics, unchanged).HF_XET_DISABLE_MEMORY_DERIVED_DOWNLOAD_BUFFERS=1(bool, unset by default) restores the static 2 GB / 512 MB / 8 GB (and static HP) values — read per-call (cheap) so it is testable withEnvVarGuard; only the probe and derived numbers are cached.info!line emitted bylogging::initafter the subscriber is installed: host total, cgroup limit, usable, and the three derived values. (It cannot be logged where the values are computed — the LazyLock is first forced while building the logging config itself, before any subscriber exists, so an event there would be silently dropped and never fire again.) Values are also visible in Python viaXetConfig.__repr__and per-fieldConfig: name = value (default)log lines.Risks
with_env_overridesno longer resets env-absent fields to macro defaults — documented; audited zero behavioral change for existing callers.HF_XET_HPand explicit buffer env vars previously got HP values; now env wins (the intended fix, but a behavior change).Testing
cargo test -p xet-runtime(232 passed) andcargo test -p xet-data(340 passed), zero failures;cargo clippyclean on both libs;hf_xetand both wasm sub-workspaces (hf_xet_thin_wasm,hf_xet_wasmon wasm32) compile, exercising the wasm stub path.cfg(not(target_family = "wasm"))target dependency ofxet_runtime, so wasm builds no longer compile it at all (cargo tree --target wasm32-unknown-unknownshows zero sysinfo entries; all three consumers —system_monitor,logging::init, the memory probe — were already wasm-gated). Side benefit: wasm consumers are not subject to sysinfo's 1.95 MSRV.Note for downstream
huggingface_hubCI or downstream tests asserting the literal 2GB/512MB/8GB defaults will see machine-derived values;HF_XET_DISABLE_MEMORY_DERIVED_DOWNLOAD_BUFFERS=1restores the static defaults. User-facing documentation for the changed env vars is updated in huggingface/hub-docs#2731 (draft until this ships in anhf_xetrelease).This implementation was AI-assisted (design and code reviewed and directed by @rajatarya).
🤖 Generated with Claude Code
https://claude.ai/code/session_01MGuU1K7rpjFfBGrA3SvCxL
Note
Medium Risk
Machine-dependent defaults may surprise downstream tests; config ordering and HP/env interaction changed. Memory sizing affects OOM risk in small containers and in-flight I/O on large hosts, though env vars and a kill switch remain.
Overview
Reconstruction download buffer defaults (
HF_XET_RECONSTRUCTION_DOWNLOAD_BUFFER_*) are no longer fixed 2 GB / 512 MB / 8 GB. They are computed from usable memory (min of host RAM and effective cgroup limit via sysinfo 0.39Process::cgroup_limits), with floors/ceilings so ~32 GB hosts match old defaults and large hosts match the old HP preset.HF_XET_DISABLE_MEMORY_DERIVED_DOWNLOAD_BUFFERS=1restores the static values.Config behavior changes:
XetConfig::new()applies defaults → high performance → env overrides so explicit env vars beatHF_XET_HP;with_env_overrideskeeps preset-adjusted fields when an env var is absent.normalize()raisesdownload_buffer_limitto at leastdownload_buffer_size(inXetConfig::newandXetContext::new);XetCommonavoids panics on incoherent configs.Infra: Rust 1.94.1 → 1.95.0 in CI/release workflows;
sysinfois a non-wasm-only dependency ofxet_runtime. Lockfiles pick up transitive updates (e.g.chacha20, objc2 stack for sysinfo).Reviewed by Cursor Bugbot for commit 242f99a. Bugbot is set up for automated code reviews on this repo. Configure here.