Repository navigation
Conversation
5576e4f to
63faf06
Compare
|
@fangyuchu Thanks for the feedback!
In principle this should not be possible under normal circumstances but we did have this bug where, during a dummy run, we were accidentally writing NaNs into the KV$. See PR more info but specifically into slot 0. With slot 0 being the sentinel slot, later requests didn't have NaNs in their logits immediately. As long as the request eventually picks up a NaN-containing block, NaNs should make their way into the logits. Therefore I don't think this race should be a cause for concern unless there is a way to avoid it without paying a large performance penalty (is there?). Ultimately, one could disable async scheduling in cases where the logic in the PR isn't able to recover model accuracy.
Maybe we do this in a follow-up PR? I'd be happy to work on this. |
| enable_nan_fault_tolerance: bool = False | ||
| """When enabled, requests producing NaN logits are immediately aborted and | ||
| KV cache blocks are zeroed on reuse to prevent corruption from propagating | ||
| into prefix cache, CPU/disk offloading tiers, or disaggregated decode | ||
| instances. Implies --enable-detect-nans-in-logits.""" |
There was a problem hiding this comment.
Move this to config/fault_tolerance.py?
| @@ -1184,7 +1183,7 @@ def record( | |||
|
|
|||
| if iteration_stats is None: | |||
| return | |||
| if envs.VLLM_COMPUTE_NANS_IN_LOGITS: | |||
| if self.vllm_config.observability_config.enable_detect_nans_in_logits: | |||
There was a problem hiding this comment.
nit: could make this flag a dedicated field
| @@ -1360,6 +1360,7 @@ def get_kv_cache_config_from_groups( | |||
| num_blocks=1, | |||
| kv_cache_tensors=[], | |||
| kv_cache_groups=kv_cache_groups, | |||
| force_zeroing=(vllm_config.observability_config.enable_nan_fault_tolerance), | |||
| @@ -1419,6 +1420,7 @@ def get_kv_cache_config_from_groups( | |||
| num_blocks=num_blocks, | |||
| kv_cache_tensors=kv_cache_tensors, | |||
| kv_cache_groups=kv_cache_groups, | |||
| force_zeroing=(vllm_config.observability_config.enable_nan_fault_tolerance), | |||
| # Collect this request's blocks so they can be evicted from | ||
| # the prefix cache below. Prefill commits full blocks to the | ||
| # prefix cache at schedule time (before the forward writes | ||
| # KV), so a block holding NaN KV may already be matchable; | ||
| # the plain free() path leaves it in the cache map. |
There was a problem hiding this comment.
Maybe reduce the size of the comment here
| # Evict NaN-corrupted blocks from the prefix cache before freeing the | ||
| # aborted requests, so a later request with the same prefix cannot match | ||
| # (and read) the NaN KV as a cache hit. Eviction resets the block hash, | ||
| # which also makes free() prioritize these blocks for reallocation, where | ||
| # the existing zero-on-allocation path wipes their contents. This mirrors | ||
| # the KV-load-failure recovery path, which evicts corrupt blocks for the | ||
| # same reason. |
There was a problem hiding this comment.
Maybe trim this block of text down a bit
There was a problem hiding this comment.
yea needs a human touch :)
| @@ -1723,6 +1713,8 @@ def update_from_output( | |||
| # to avoid expensive operations inside the loop. | |||
| stopped_running_reqs: set[Request] = set() | |||
| stopped_preempted_reqs: set[Request] = set() | |||
| nan_abort_req_ids: set[str] = set() | |||
| nan_block_ids: set[int] = set() | |||
There was a problem hiding this comment.
clearer?
| nan_block_ids: set[int] = set() | |
| nan_block_ids_to_evict: set[int] = set() |
… abort Add two CLI flags for NaN corruption handling: - --enable-detect-nans-in-logits: enables NaN counting in logits per step (replaces VLLM_COMPUTE_NANS_IN_LOGITS env var with a proper CLI flag while keeping backward compat) - --enable-nan-fault-tolerance: implies the above, and additionally aborts requests when NaN logits are detected (during chunked prefill or decode) and forces KV cache block zeroing on reuse to prevent corruption from propagating into prefix cache, CPU/disk offloading tiers, or disaggregated decode instances via NIXL Signed-off-by: Elvir Crncevic <elvir@crncevic.dev> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
The NaN detection path was only wired into the legacy (V1) GPU model runner. The default V2 runner (vllm/v1/worker/gpu/model_runner.py) constructed its Sampler without detect_nans_in_logits, so num_nans was never computed and num_nans_in_logits never populated on the model runner output. The async output plumbing already forwards num_nans to ModelRunnerOutput; only the sampler flag was missing. Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
When a request is aborted for NaN in logits, its KV blocks are freed via the normal path, which returns them to the pool but leaves them in the prefix-cache hash map. Prefill commits full blocks to the prefix cache at schedule time (before the forward writes KV), so a block holding NaN KV can remain matchable and be reused as a prefix-cache hit by a later request with the same prefix -- bypassing the zero-on-reuse path, since cache hits are never re-zeroed. Evict the aborted request's blocks from the prefix cache before freeing them, mirroring the KV-load-failure recovery path. Eviction resets the block hash so free() also prioritizes the blocks for reallocation, where the existing zero-on-allocation logic wipes their contents. Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
…oleranceConfig - Move enable_nan_fault_tolerance from ObservabilityConfig to FaultToleranceConfig - Rename nan_block_ids -> nan_block_ids_to_evict in scheduler - Trim verbose comments in scheduler NaN abort/eviction paths - Cache detect_nans_in_logits as dedicated field in stat loggers - Remove redundant parens in kv_cache_utils force_zeroing - Update tests to use new config path Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Delay prefix-cache publication until asynchronous outputs have been validated, aggregate speculative NaN counts per request without host synchronization, and preserve corruption metrics on abort. Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Cover sync and async scheduling across eager and CUDA graph execution, plus multi-row MTP verification, request abort, prefix-cache isolation, recovery, and corruption metrics on one small Qwen checkpoint. Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Co-authored-by: Codex <codex@openai.com> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Assisted-by: Codex Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Initialize aggregation buffers on GPU and leave MRV1 unchanged. Assisted-by: Codex (OpenAI) Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Reduce per-row counts per request in one kernel and cover empty, long, and strided ranges. Assisted-by: Codex (OpenAI) Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Add short examples for regular and empty request row ranges. Assisted-by: Codex (OpenAI) Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Assisted-by: Codex (OpenAI) Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Read fault tolerance settings from ParallelConfig. Assisted-by: Codex (OpenAI) Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Check replay size against the runner decode length, including speculative decoding. Assisted-by: Codex (OpenAI) Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Retain earlier validated GPU prefixes when a later chunk fails. Delay OffloadingConnector stores and SimpleCPU eager cursors until output validation, and keep async Mamba checkpoints behind the same boundary. Consolidate the offload fix from fork PR #5 into vllm-project#50283. Cover sync/async eager/lazy stores, late acknowledgements, and checkpoint publication. Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
Verify the upstream rebase keeps CLI-enabled NaN detection visible in request metrics and does not clear corruption on a later clean output. Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Elvir Crncevic <elvircrn@gmail.com>
e7d79bd to
d51247d
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
Overview
Add
--enable-nan-fault-tolerancefor MRV2: abort on NaN logits and withhold the unchecked chunk from GPU prefix reuse and CPU offloading. Earlier validated prefixes stay reusable.This consolidates fork PR #5 into this PR. The other open NaN fixes address individual kernels; this adds engine-level recovery.
Claims
--enable-detect-nans-in-logitsexposes detection as an engine argument, retaining the legacy environment variable. Fault tolerance implies detection and requires MRV2.OffloadingConnectorand eagerSimpleCPUOffloadConnectorstores until validation. SimpleCPU lazy mode already selects published GPU blocks. No additional GPU-to-CPU status readbacks or KV scans.Validation
Rebased onto upstream main
0c2584929. Preserves upstream speculative-stop accounting and offload-manager locking; NaN metrics work with engine arguments even when the legacy environment flag is off.CPU-only scheduler regressions cover sync/async prefill, first/later chunk failures, eager/lazy offloading, late copy acknowledgements, clean completion, and hybrid checkpoint publication. The new regressions fail on the pre-fix head (
ab687e51e, 20 failures).739 passed, 2 skipped across the affected scheduler, cache-manager, offload-worker, CPU-manager, and tier-manager suites. All applicable pre-commit hooks passed, including Ruff, mypy, and CI test tethering. Tests used a CPU-only harness to permit MRV2 configuration without CUDA; no model or GPU kernels ran.
CPU test coverage
tests/v1/simple_kv_offload/test_scheduler.pytests/v1/core/test_single_type_kv_cache_manager.pytests/v1/kv_connector/unit/offloading_connector/{test_scheduler,test_worker}.pytests/v1/kv_offload/{cpu/test_manager,tiering/test_tiering_offloading}.pytests/v1/metrics/test_stats.pytests/v1/core/{test_scheduler,test_async_scheduler}.pyandtests/v1/streaming_input/test_scheduler_streaming.py, filtered to NaN, stop, local-prefill, and throttle checksThe CUDA suite now includes eager/lazy SimpleCPU offloading. The updated CUDA end-to-end cases,
VLLM_GPU_SYNC_CHECK=error, and model evaluations have not been run on this macOS host. Previous GPU results do not validate this update.Details
Scheduling advances computed-token counts before outputs arrive. Stores now stop at the completed-token boundary; GPU publication already waits for the logits check. A failed chunk stays unpublished, while earlier validated blocks can finish offloading and remain cache hits. Retries recompute the missing suffix normally.
Known limitations
Pull Request Checklist
/pr-checklistskill.