fix(grpc_servicer): handle vllm log forwarding on servicer side - #975
Conversation
Attach the smg_grpc_servicer parent logger to the vllm logging hierarchy in the vllm subpackage __init__, so vllm's grpc_server no longer needs to configure it. Also switch health_servicer to use vllm's init_logger for consistency with servicer.py. Signed-off-by: Chang Su <chang.s.su@oracle.com>
📝 WalkthroughWalkthroughCentralized logging setup added: the package logger for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request integrates the smg_grpc_servicer logging with the vllm logging hierarchy and updates the health servicer to use vllm.logger.init_logger. A suggestion was made to copy the logger handlers instead of referencing them directly to avoid unintended side effects from future modifications to the shared handler list.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grpc_servicer/smg_grpc_servicer/vllm/__init__.py`:
- Around line 11-15: The current logger setup aliases _pkg_logger.handlers to
_vllm_logger.handlers and unconditionally sets _pkg_logger.propagate = False;
instead, copy _vllm_logger.handlers into _pkg_logger.handlers (so changes to
vllm handlers later don't affect our logger) and only disable propagation when
there are handlers present on _pkg_logger (i.e., set propagate=False if the
copied handlers list is non-empty, otherwise leave propagation unchanged) —
update the initialization that references _vllm_logger and _pkg_logger to
perform a shallow copy of handlers and a conditional propagate assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 458bc0f1-e452-4604-b4c2-60e0704334eb
📒 Files selected for processing (2)
grpc_servicer/smg_grpc_servicer/vllm/__init__.pygrpc_servicer/smg_grpc_servicer/vllm/health_servicer.py
Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
grpc_servicer/smg_grpc_servicer/vllm/__init__.py (1)
15-15:⚠️ Potential issue | 🟠 MajorGuard
propagatebased on handler availability.
_pkg_logger.propagate = Falseis still unconditional. Ifvllmhas zero handlers at import time,smg_grpc_servicerlogs can be suppressed. SetpropagatetoFalseonly when copied handlers are present.Suggested fix
_pkg_logger.handlers = list(_vllm_logger.handlers) _pkg_logger.setLevel(_vllm_logger.level) -_pkg_logger.propagate = False +if _pkg_logger.handlers: + _pkg_logger.propagate = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc_servicer/smg_grpc_servicer/vllm/__init__.py` at line 15, The package logger currently unconditionally sets _pkg_logger.propagate = False which can suppress host app logs if vllm has no handlers; change the logic in vllm/__init__.py where _pkg_logger handlers are copied so that you only set _pkg_logger.propagate = False when there are copied/attached handlers (e.g., check if _pkg_logger.handlers or the list returned by copy of handlers is non-empty) and leave propagate True otherwise; update the block that copies handlers and the propagate assignment (referencing _pkg_logger and the handler-copying code) so propagate is conditional on handlers being present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@grpc_servicer/smg_grpc_servicer/vllm/__init__.py`:
- Line 15: The package logger currently unconditionally sets
_pkg_logger.propagate = False which can suppress host app logs if vllm has no
handlers; change the logic in vllm/__init__.py where _pkg_logger handlers are
copied so that you only set _pkg_logger.propagate = False when there are
copied/attached handlers (e.g., check if _pkg_logger.handlers or the list
returned by copy of handlers is non-empty) and leave propagate True otherwise;
update the block that copies handlers and the propagate assignment (referencing
_pkg_logger and the handler-copying code) so propagate is conditional on
handlers being present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4b058ff-98f8-418f-93fa-436a6f29b599
📒 Files selected for processing (1)
grpc_servicer/smg_grpc_servicer/vllm/__init__.py
…project#975) Signed-off-by: Chang Su <chang.s.su@oracle.com>
Description
Problem
vllm's
grpc_server.pymanually attaches thesmg_grpc_servicerlogger to the vllm logging hierarchy at module level. This means vllm has to know about and configure third-party loggers, which was flagged in vllm-project/vllm#38333 (comment).Solution
Move the logger attachment into the
smg_grpc_servicer.vllmsubpackage__init__.pyso it happens automatically when the servicer is imported. Also switchhealth_servicer.pyfromlogging.getLoggertovllm.logger.init_loggerfor consistency withservicer.py.Changes
grpc_servicer/smg_grpc_servicer/vllm/__init__.py: Attach the top-levelsmg_grpc_servicerlogger to the vllm logging hierarchy on import.grpc_servicer/smg_grpc_servicer/vllm/health_servicer.py: Usevllm.logger.init_loggerinstead oflogging.getLogger.Test Plan
--grpcand send requestsGenerate request ...) still appear with vLLM's log formatChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit