Repository navigation
[Bugfix] Avoid retaining tensors in Quack FP8 fallback logging - #9262
princepride merged 3 commits into
Conversation
Signed-off-by: Rahul Steiger <rsteiger@nvidia.com>
Signed-off-by: Rahul Steiger <rsteiger@nvidia.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/quantization.md. Module owners: @david6666666 @Isotr0py @lishunyang12 Routing: @david6666666 via module of the changed files, module named in the PR description, CODEOWNERS; @Isotr0py via module of the changed files, module named in the PR description, CODEOWNERS; @lishunyang12 via module of the changed files, module named in the PR description, CODEOWNERS @rahul-steiger-nv, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
Dong1017
left a comment
There was a problem hiding this comment.
Independent verification at this revision: the targeted CPU suite passed all 15 tests. As a negative control, I kept the head test file unchanged and substituted only the production module from base 798fef99a33fba3ad3c2439d6ec10fe838c0076b; the new regression then failed specifically at the tensor-release assertion.
This used the real vLLM 0.32 cached logger. Its cache retains the warning arguments before formatting, so converting exc to a string at the call site removes the cache's reference to the exception and traceback while preserving the FlashInfer fallback. I found no actionable issue in this diff. The tests use the repository's existing GEMM/fallback doubles to check Python object lifetime; I did not measure GPU memory, kernel execution or performance.
Omni ReviewBot: supersededThe CI failure noted on |
|
Thanks for the review @princepride. I looked at the failing CI and it seems to be unrelated to my changes. I merged my PR with main. Would it be possible for you to retrigger CI? |
Contributor intent (required human answers)
1. What outcome do you want to achieve?
Fix the memory leak.
2. What is the highest-leverage variable for that outcome?
Avoid retaining GPU memory after Quack failures.
3. How will this work move that variable?
Pass the exception string instead of the exception object to the cached logger.
4. Whose customer or user needs does this address, and what are those needs?
Users encountering Quack failures need inference to continue without this memory leak.
Purpose
Quack FP8 fallback logging passes an exception to
warning_once, whose cache retains its traceback and tensors. Format the exception before logging so failed GEMMs release their tensors while preserving FlashInfer fallback behavior.Adds a CPU regression using the real cached logger. APIs, configuration, and quantization behavior are unchanged; documentation is unchanged. Related: vllm-project/vllm#54098.
Test Plan
vLLM Version: Supplied by the vLLM-Omni v0.31.0rc1 ARM64 container
vLLM-Omni Commit: f7d10d9 (base: 798fef9).
python -m pytest tests/quantization/test_quack_fp8_runtime.py -m 'core_model and cpu' --run-level core_model -q -p no:cacheproviderTest Result
AI assistance: Codex investigated the issue, implemented the fix and regression test, ran validation, and drafted this description.