Deprecate --disable-log-requests and replace with --enable-log-requests
#21739
Conversation
…quests` Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
hmellor
requested review from
WoosukKwon,
aarnphm,
alexm-redhat,
comaniac,
njhill,
robertgshaw2-redhat and
ywang96
as code owners
|
This pull request has merge conflicts that must be resolved before it can be |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly flips the default behavior for request logging and replaces --disable-log-requests with --enable-log-requests. The changes are consistent across configuration, tests, and application code. The use of a deprecated property for backward compatibility is well-handled. I've found one critical issue with the implementation of the deprecation warning which could lead to a crash, and I've provided a suggestion to fix it.
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
|
👋 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 🚀 |
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
…uests` (vllm-project#21739) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
|
This PR is not fully backward compatible from async engine arg perspective @hmellor . please see ray-project/ray#55314 |
|
You can't check if it's an attr anymore because it's been replaced with a property. It is still gettable ( |
|
yeah but our usage was |
|
I see, so the issue is that it's no longer in the |
…uests` (vllm-project#21739) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Signed-off-by: Jinzhen Lin <linjinzhen@hotmail.com>
…uests` (vllm-project#21739) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Signed-off-by: Noam Gat <noamgat@gmail.com>
…uests` (vllm-project#21739) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Signed-off-by: Paul Pak <paulpak58@gmail.com>
…uests` (vllm-project#21739) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Signed-off-by: Diego-Castan <diego.castan@ibm.com>
…uests` (vllm-project#21739) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
…uests` (vllm-project#21739) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
4 participants
This PR flips the default behaviour so that logging of requests is disabled by default for security/privacy reasons.
It then replaces the
--disable-log-requestswith--enable-log-requestsfor a better UX for opting in to logging requests.