-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
[Bugfix] Disable the statslogger if the api_server_count is greater than 1 #22227
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
Conversation
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.
Code Review
This pull request introduces a bugfix to disable the statistics logger when api_server_count is greater than 1, as this configuration is not compatible. The change correctly saves the original state of the disable_log_stats argument, forces it to True when multiple API servers are used, and issues a warning to the user if the feature was previously enabled. The implementation is consistent with how other incompatible features are handled in this scenario. The code is correct and effectively addresses the issue.
|
Hi, @njhill I have a question: when vllm/vllm/engine/llm_engine.py Lines 343 to 368 in 601f856
|
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
@chaunceyjiang yes that's expected. Here we only want to omit The API here is kind of crappy in general - if you provide custom stats loggers then they will replace the built-in ones rather than augment them. There's a separate discussion / PR proposal around that I think. |
|
/cc @njhill PTAL. |
|
@chaunceyjiang I wonder if you could rebase this now that #20952 is merged? The behaviour now is that custom stats loggers will augment the built-in ones unless |
a42c8a7 to
1529366
Compare
…han 1 Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
1529366 to
9d353fa
Compare
|
@njhill PTAL. |
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.
Thanks @chaunceyjiang.
Actually we still want the prometheus metrics in this case and I think this change will disable those too.
I think the check should go here instead:
vllm/vllm/v1/metrics/loggers.py
Lines 662 to 663 in eedb2a2
| if enable_default_loggers and logger.isEnabledFor(logging.INFO): | |
| factories.append(LoggingStatLogger) |
…han 1 Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…han 1 Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…han 1 Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
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.
Thanks @chaunceyjiang
|
Remaining test failure is also occurring on main. |
…han 1 (vllm-project#22227) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com> Co-authored-by: Nick Hill <nhill@redhat.com>
…han 1 (vllm-project#22227) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com> Co-authored-by: Nick Hill <nhill@redhat.com>
…han 1 (vllm-project#22227) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com> Co-authored-by: Nick Hill <nhill@redhat.com>
…han 1 (vllm-project#22227) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com> Co-authored-by: Nick Hill <nhill@redhat.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
…han 1 (vllm-project#22227) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com> Co-authored-by: Nick Hill <nhill@redhat.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Fixes #21954
Purpose
Test Result
(Optional) Documentation Update