feat(cli): preflight host resource limits before workers (#234) - #320
Closed
Inoriac wants to merge 7 commits into
Closed
feat(cli): preflight host resource limits before workers (#234)#320Inoriac wants to merge 7 commits into
Inoriac wants to merge 7 commits into
Conversation
Probe file-descriptor (RLIMIT_NOFILE), process-count (RLIMIT_NPROC), and
shared-memory (kernel.shmmax) limits via the stdlib and compare them against
a documented demand estimate for world_size/tp_size before train/serve spawn
workers. Reuse the diagnostics CheckResult severity model.
Add --resource-check {skip,warn,block} to train and serve with a safe warn
default: emit a stderr diagnostic only on a FAIL and never abort, so existing
runs and stdout stay backward compatible; block aborts before worker init on
a below-demand probe. Probes unavailable on a platform (e.g. macOS shmmax)
degrade to a non-blocking WARN; RLIM_INFINITY counts as meeting demand.
Co-Authored-By: Claude <noreply@anthropic.com>
Describe the --resource-check option on train and serve, the fd/process/shm probe contract, the demand estimate, OK/WARN/FAIL severities, output fields, platform degradation, and a minimal runnable example covering success and a boundary failure path. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a host “resource preflight” to areno train / areno serve that probes fd / process / shared-memory limits before spawning worker ranks, compares them to an estimated demand for world_size / tp_size, and reports via the existing CheckResult OK/WARN/FAIL model (with optional blocking via a new CLI flag).
Changes:
- Introduces host limit probing + demand estimation + rendering utilities in
areno/cli/diagnostics.py. - Wires a new
--resource-check {skip,warn,block}option into bothareno trainandareno servepre-init flows. - Adds CPU tests and user documentation describing the new checks, policies, and output.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
areno/cli/diagnostics.py |
Adds limit probes, demand estimation, CheckResult generation, and formatting helpers for the resource preflight. |
areno/cli/train.py |
Adds --resource-check option and runs the preflight before model/backend init in the training CLI. |
areno/cli/serve.py |
Adds --resource-check option and runs the preflight before model resolution / app creation in the serving CLI. |
tests/test_resource_preflight_cpu.py |
Adds deterministic CPU tests for demand math, severity mapping, formatting, and train/serve wiring via mocks. |
docs/cli/diagnostics.rst |
Documents probes, policies, severities, and example output for the resource preflight. |
docs/cli/training.rst |
Documents the new --resource-check option for areno train. |
docs/cli/inference.rst |
Documents the new --resource-check option for areno serve. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+598
to
+616
| - file descriptors: per-worker base plus one socket per cross-rank peer for | ||
| the NCCL/tensor-parallel mesh; sum across all `world_size` workers. | ||
| - processes: `world_size` worker ranks plus the driver process. | ||
| - shared memory: baseline per tensor-parallel group, scaled by `tp_size`. | ||
|
|
||
| The estimate is intentionally conservative -- it triggers a warning rather | ||
| than undershooting a real NCCL/CUDA IPC requirement mid-run. | ||
| """ | ||
|
|
||
| if world_size < 1: | ||
| raise ValueError("world_size must be >= 1") | ||
| if tp_size < 1: | ||
| raise ValueError("tp_size must be >= 1") | ||
| cross_rank_peers = world_size * (world_size - 1) | ||
| return { | ||
| "file_descriptors": _BASE_FDS_PER_WORKER * world_size + cross_rank_peers, | ||
| "processes": world_size + 1, | ||
| "shared_memory": _SHM_BASELINE_BYTES * tp_size, | ||
| } |
Comment on lines
+638
to
+642
| if policy not in {"skip", "warn", "block"}: | ||
| raise ValueError(f"resource-check policy must be skip/warn/block, got {policy!r}") | ||
| if limits is None: | ||
| limits = collect_host_limits() | ||
| demand = estimate_resource_demand(world_size, tp_size) |
Comment on lines
+670
to
+677
| def _shmmax_result(observed: dict[str, Any], required: int) -> CheckResult: | ||
| return _resource_result( | ||
| observed, | ||
| required, | ||
| name="shared memory (kernel.shmmax)", | ||
| unit=" bytes", | ||
| adjust="raise the system limit, e.g. `sudo sysctl -w kernel.shmmax=<required>`", | ||
| ) |
Address review feedback on the host-resource preflight: - estimate_resource_demand: fd demand was summed across the whole fleet (_BASE_FDS_PER_WORKER * world + world*(world-1)) but compared against RLIMIT_NOFILE, which is a per-process limit. Use the per-process demand (base + (world_size - 1) peers) instead. The processes estimate stays as the fleet total since RLIMIT_NPROC is per-user. This removes false FAILs / spurious ulimit guidance at larger world_size. - preflight_host_resources: short-circuit on policy="skip" without probing the host, so the function's skip semantics match its callers. - _shmmax_result: interpolate the concrete required value into the sysctl remediation hint instead of leaving a <required> placeholder. Tests updated for the new fd formula and the shm hint; added a case asserting skip never touches collect_host_limits. Co-Authored-By: Claude <noreply@anthropic.com>
- serve.py: reorder diagnostics import ahead of model_refs (I001) - test_resource_preflight_cpu.py: drop unused `result` binding (F841), add trailing newline (W292) No behavior change; ruff check passes clean on the changed files. Co-Authored-By: Claude <noreply@anthropic.com>
Wrap the over-long _limits(...) call across multiple lines to satisfy ruff-format. Auto-applied by pre-commit run --all-files. Co-Authored-By: Claude <noreply@anthropic.com>
…preflight-234 # Conflicts: # areno/cli/serve.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a preflight that reads host file-descriptor, process-count, and
shared-memory limits before
areno train/areno servespawn workerranks, compares them against a documented demand estimate for
world_size/tp_size, and reports normal / warning / blocking severitieswithout changing the host.
Closes #234. Follows my initial sketch in the issue: reuse AReno's existing
preflight paradigm, probe limits via the stdlib, estimate demand from
concurrency scale, and report severities with the specific delta and
remediation guidance.
How
Reuses the existing diagnostics
CheckResult(status, name, detail, next_step)severity model (OK/WARN/FAIL) rather than introducing a parallel subsystem.
Probes (
areno/cli/diagnostics.py:collect_host_limits):resource.getrlimit(RLIMIT_NOFILE)resource.getrlimit(RLIMIT_NPROC)/proc/sys/kernel/shmmax(Linux)Demand estimate (
estimate_resource_demand, conservative upper bound):64 * world_size + world_size * (world_size - 1)world_size + 11 GiB * tp_sizeNew CLI option
--resource-check {skip,warn,block}on bothtrainandserve, defaultwarn:warn(default) — emit a stderr diagnostic only when a probed limit isbelow demand, then continue. Never aborts. Existing stdout / runs are
unchanged.
block— raiseUsageErrorand abort before any worker starts on abelow-demand probe.
skip— disable.Severities:
OK— observed meets demand.RLIM_INFINITYcounts as meeting demand.WARN— probe unavailable on this platform (e.g. macOS/Windows have no/proc/sys/kernel/shmmax). Never blocks, even underblock.FAIL— observed below demand; detail carries exactobserved/required/deltaand a remediation hint(
ulimit -n 65536,sudo sysctl -w kernel.shmmax=...).Backward compatibility: preflight output goes to stderr and is only
printed on a FAIL, so stdout (including the machine-parsed config summary) is
byte-for-byte unchanged on the success path; existing CLI behavior is
preserved under the default.
No new dependencies, no config-dataclass changes (the option lives at the CLI
layer with explicit inputs), no external database/sandbox.
Testing
New
tests/test_resource_preflight_cpu.py(16 cases, all in-k cpu) coversulimit -n; low shm → FAIL +kernel.shmmaxRLIM_INFINITY→ OK (not unavailable)blockshould_block_on_resourcescounts only FAILwarndoes not abort on FAIL;blockaborts before
run()/create_app();skipdoes not invoke the probeFull CPU suite green:
pytest tests/ -k cpu→ 370 passed, no regressionin
test_cli_diagnostics_cpu.py/test_train_cli_config_cpu.py/test_serve_cli_cpu.py.GPU train/serve launch not exercised here (requires CUDA); orchestration logic
is covered via CPU tests + mocks.
Issue #234 acceptance
and adjustment guidance; degrade cleanly on platforms without a probe.
CheckResult); no external DB or sandbox.aborts; stdout unchanged).
boundary/failure path.
output (
docs/cli/diagnostics.rst,training.rst,inference.rst).Commit history
feat(cli): preflight host resource limits before workersdocs(cli): document host resource preflight option