LCORE- Unify logging configuration and setup#1703
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR centralizes logging into a cached setup_logging() that merges custom config into Uvicorn's LOGGING_CONFIG, adds DEFAULT_LOGGER_NAME and a new plain-text format, invokes setup during module import and in the Uvicorn runner, and updates tests to exercise and reset the new flow. ChangesLogging Setup Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runners/uvicorn.py (1)
17-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring doesn't document the new
log_configparameter🛠 Proposed fix
"""Start the Uvicorn server using the provided service configuration. Parameters: ---------- configuration (ServiceConfiguration): Configuration providing host, port, workers, and `tls_config` (including `tls_key_path`, `tls_certificate_path`, and `tls_key_password`). TLS fields may be None and will be forwarded to uvicorn.run as provided. + log_config (dict | None): Logging configuration dict passed to uvicorn.run. + When None, the centralized setup_logging() result is used. """As per coding guidelines, all functions must "include descriptive docstrings" covering all parameters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runners/uvicorn.py` around lines 17 - 25, The docstring for the Uvicorn-starting function (start_uvicorn in src/runners/uvicorn.py) is missing documentation for the new log_config parameter; update the function docstring to list and describe log_config (type: optional dict or logging config path, default/behavior: forwarded to uvicorn.run to configure logging) alongside the existing parameters so callers know what to pass and how it is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/constants.py`:
- Line 228: DEFAULT_LOGGER_NAME is missing the Final[str] annotation; update the
constant declaration for DEFAULT_LOGGER_NAME to be annotated with Final[str]
(e.g., DEFAULT_LOGGER_NAME: Final[str] = "lightspeed_stack") and ensure Final is
imported from typing at the top of src/constants.py (add "from typing import
Final" if it's not already present) so the constant is type-locked like the
others.
In `@src/log.py`:
- Line 1: The file src/log.py fails Black formatting; run the project formatter
and commit the changes: execute "uv tool run black src/log.py" (or "uv tool run
black src tests") to reformat the module (src.log) and then stage and commit the
updated file so the CI Black check passes.
- Around line 59-70: The logger helper uses a DEFAULT_LOGGER_NAME prefix to work
around inconsistent __name__ values across different entrypoints (see get_logger
and DEFAULT_LOGGER_NAME); open a tracking issue to remove that workaround by
standardizing the package/entrypoint layout so __name__ always yields the
expected root, document the change, and include a migration plan: update
get_logger to use __name__ (remove the DEFAULT_LOGGER_NAME concatenation),
refactor any callers that relied on the prefixed logger names (adjust tests and
configuration), and add a small integration test that verifies the root logger
name is consistent when run via all supported entrypoints.
- Around line 107-117: The setup_logging() function currently calls
deep_update(uvicorn.config.LOGGING_CONFIG, logging_conf) then mutates
merged_config which aliases uvicorn.config.LOGGING_CONFIG; instead construct all
conditional changes (the "rich" handler branch and the access/default formatter
entries: the handlers for "uvicorn" and "uvicorn.access" and the
formatters["access"] and formatters["default"]["fmt"/"datefmt"]) inside
logging_conf before calling deep_update so the single deep_update call merges
everything and merged_config is never mutated afterwards; update the code paths
that reference handler, merged_config and logging_conf accordingly so no
post-merge assignments (e.g., merged_config["formatters"][...] = ...) remain.
- Line 89: The Rich handler configuration uses "log_time_format" set to
"%Y-%m-%d %H:%M:%S.%f" which emits 6-digit microseconds instead of the intended
3-digit milliseconds; update the Rich-based logging path to remove the "%f"
subsecond directive (or omit log_time_format entirely) and rely on RichHandler's
own millisecond column via show_time=True, ensuring consistency with
DEFAULT_LOG_FORMAT which uses "%(msecs)03d"; modify the code that sets
log_time_format (symbol: log_time_format) and the Rich handler instantiation
(symbol: RichHandler, show_time) so the Rich path does not output 6-digit
microseconds.
- Line 11: The import of pydantic.v1.utils.deep_update in src/log.py uses an
internal v1-compat API — remove that import, add a small local helper function
named _deep_update(base: dict, override: dict) that recursively merges override
into base (preserving dict semantics), and replace all usages of
deep_update(...) with _deep_update(...); update any function or module
references in this file that call deep_update to call _deep_update instead so
there is no dependency on pydantic internals.
- Around line 73-74: Add a clear docstring to the public function
setup_logging() that explains what the function does (e.g., configures and
returns the logging configuration dict, applies any global logging settings or
handlers) and any side effects (such as calling logging.config.dictConfig or
mutating global logger state); ensure the docstring appears immediately below
the def setup_logging() line and follows project docstring style so pydocstyle
D103 is satisfied.
---
Outside diff comments:
In `@src/runners/uvicorn.py`:
- Around line 17-25: The docstring for the Uvicorn-starting function
(start_uvicorn in src/runners/uvicorn.py) is missing documentation for the new
log_config parameter; update the function docstring to list and describe
log_config (type: optional dict or logging config path, default/behavior:
forwarded to uvicorn.run to configure logging) alongside the existing parameters
so callers know what to pass and how it is used.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c2b1d91-2f32-49b8-ae1e-1cede9a1feb3
📒 Files selected for processing (5)
src/constants.pysrc/lightspeed_stack.pysrc/log.pysrc/runners/uvicorn.pytests/unit/test_log.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: unit_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/constants.pysrc/lightspeed_stack.pysrc/runners/uvicorn.pysrc/log.py
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
constants.pyfor shared constants with descriptive comments and type hints usingFinal[type]
Files:
src/constants.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/test_log.py
🪛 GitHub Actions: Black / 0_black.txt
src/log.py
[error] 1-1: Black formatting check failed (uv tool run black --check src tests). File would be reformatted. Run 'uv tool run black src tests' (or 'black --write') to fix.
🪛 GitHub Actions: Black / black
src/log.py
[error] 1-1: Black --check failed: would reformat /home/runner/work/lightspeed-stack/lightspeed-stack/src/log.py (1 file would be reformatted). Run 'uv tool run black src/log.py' (or 'uv tool run black --write src tests') to apply formatting.
🪛 GitHub Actions: Pydocstyle / 0_pydocstyle.txt
src/log.py
[error] 74-74: pydocstyle failed (D103): Missing docstring in public function setup_logging.
🪛 GitHub Actions: Pydocstyle / pydocstyle
src/log.py
[error] 74-74: pydocstyle (uv tool run pydocstyle -v src tests) reported: D103: Missing docstring in public function setup_logging.
🔇 Additional comments (2)
src/lightspeed_stack.py (1)
13-19: LGTM — logging initialization order is correct
setup_logging()is called beforeget_logger(__name__), ensuringdictConfigis applied before the logger hierarchy is first accessed.tests/unit/test_log.py (1)
14-16: The fixture is working correctly.deep_updatefrompydantic.v1.utilscreates a new merged dictionary rather than mutatinguvicorn.config.LOGGING_CONFIGin-place, sosetup_logging()does not leave persistent global state mutations across tests. Clearing the@lru_cacheensures the function body re-executes for each test, which is the appropriate behavior since tests modify environment variables viamonkeypatchbetween runs.
samdoran
force-pushed
the
use-existing-logger
branch
from
113ef13 to
b321e1d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runners/uvicorn.py (1)
13-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate docstring to document the new
log_configparameter.The function signature was updated to accept
log_config: dict | None = None, but the docstring does not document this parameter.📝 Proposed fix
"""Start the Uvicorn server using the provided service configuration. Parameters: ---------- configuration (ServiceConfiguration): Configuration providing host, port, workers, and `tls_config` (including `tls_key_path`, `tls_certificate_path`, and `tls_key_password`). TLS fields may be None and will be forwarded to uvicorn.run as provided. + log_config (dict | None): Optional logging configuration dict to pass + to uvicorn.run. If None, setup_logging() is called to generate the + unified configuration. """As per coding guidelines,
src/**/*.pyrequires "All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runners/uvicorn.py` around lines 13 - 25, The docstring for start_uvicorn is missing documentation for the new log_config parameter; update the function's docstring to add a "Parameters" entry for log_config (type dict | None) describing that it supplies an optional uvicorn logging configuration passed to uvicorn.run and that None uses default logging behavior, and ensure the return type sentence remains accurate for the function signature of start_uvicorn.
♻️ Duplicate comments (4)
src/log.py (4)
11-11: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
pydantic.v1.utils.deep_updateis an internal v1-compat shim — replace with a self-contained helper.
pydantic.v1.utilsis part of the v1 compatibility layer bundled in pydantic v2, not a public API. A 5-line replacement eliminates the dependency on pydantic internals.♻️ Proposed replacement
Remove the pydantic import and add a local helper:
-from pydantic.v1.utils import deep_updatedef _deep_update(base: dict, override: dict) -> dict: """Recursively merge override into base, returning a new dict.""" result = base.copy() for key, value in override.items(): if key in result and isinstance(result[key], dict) and isinstance(value, dict): result[key] = _deep_update(result[key], value) else: result[key] = value return resultThen replace all calls to
deep_update(...)with_deep_update(...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log.py` at line 11, The import from pydantic.v1.utils using deep_update is relying on an internal v1-compat API; remove the line importing deep_update and add a small local helper named _deep_update that recursively merges two dicts (signature _deep_update(base: dict, override: dict) -> dict), then replace all usages of deep_update(...) in this module with _deep_update(...); ensure the helper returns a new dict and performs recursive merging when both values are dicts to preserve current behavior of deep_update.
108-119:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
setup_logging()silently mutatesuvicorn.config.LOGGING_CONFIG— corrupts global state on cache replay.
deep_update(uvicorn.config.LOGGING_CONFIG, logging_conf)performs only a shallow copy at the top level. Becauselogging_confcontains no"formatters"key,merged_config["formatters"]is the same dict object asuvicorn.config.LOGGING_CONFIG["formatters"]. The subsequent assignments on lines 114–119 therefore permanently mutate uvicorn's module-level global. Similarly, line 111-112 mutate the uvicorn logger dicts.In tests this causes silent pollution:
cache_clear()re-runs the function body against an already-mutateduvicorn.config.LOGGING_CONFIG.🛠 Proposed fix
Build all conditional configuration inside
logging_confbefore the singledeep_updatecall:logging_conf = { "version": 1, "disable_existing_loggers": False, "handlers": { "rich": { "()": "rich.logging.RichHandler", "show_time": True, - "log_time_format": "%Y-%m-%d %H:%M:%S.%f", + "log_time_format": "%Y-%m-%d %H:%M:%S", "level": log_level, }, }, "loggers": { DEFAULT_LOGGER_NAME: { "handlers": [handler], "level": log_level, "propagate": False, }, "llama_stack_client": { "handlers": [handler], "level": log_level, "propagate": False, }, }, } - merged_config = deep_update(uvicorn.config.LOGGING_CONFIG, logging_conf) - if handler == "rich": - merged_config["loggers"]["uvicorn"]["handlers"] = [handler] - merged_config["loggers"]["uvicorn.access"]["handlers"] = [handler] + logging_conf["loggers"]["uvicorn"] = {"handlers": ["rich"]} + logging_conf["loggers"]["uvicorn.access"] = {"handlers": ["rich"]} else: - merged_config["formatters"]["access"]["fmt"] = ( - "%(asctime)s.%(msecs)03d %(levelprefix)s " - '%(client_addr)s - "%(request_line)s" %(status_code)s' - ) - merged_config["formatters"]["default"]["fmt"] = DEFAULT_LOG_FORMAT - merged_config["formatters"]["default"]["datefmt"] = "%Y-%m-%d %H:%M" + logging_conf["formatters"] = { + "access": { + "fmt": '%(asctime)s.%(msecs)03d %(levelprefix)s %(client_addr)s - "%(request_line)s" %(status_code)s', + }, + "default": { + "fmt": DEFAULT_LOG_FORMAT, + "datefmt": "%Y-%m-%d %H:%M", + }, + } + + merged_config = deep_update(uvicorn.config.LOGGING_CONFIG, logging_conf) logging.config.dictConfig(merged_config) return merged_config🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log.py` around lines 108 - 119, setup_logging currently calls deep_update(uvicorn.config.LOGGING_CONFIG, logging_conf) then mutates merged_config (and thus uvicorn.config.LOGGING_CONFIG) via assignments to merged_config["formatters"] and merged_config["loggers"], corrupting global state; fix by avoiding in-place mutation of uvicorn's global: either deepcopy uvicorn.config.LOGGING_CONFIG before merging (use copy.deepcopy on uvicorn.config.LOGGING_CONFIG into merged_config) or build all conditional keys into logging_conf first (populate logging_conf["formatters"] and logging_conf["loggers"] based on handler) and then call deep_update once; update references to merged_config, deep_update, logging_conf and setup_logging accordingly so no assignment mutates uvicorn.config.LOGGING_CONFIG.
90-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
log_time_formatuses%f(microseconds, 6 digits) — PR goal was milliseconds.The
%fstrftimedirective expands to 6-digit microseconds (e.g.,123456). The non-Rich path correctly uses%(msecs)03dfor 3-digit milliseconds inDEFAULT_LOG_FORMAT. The Rich handler will show 6-digit precision instead of the intended 3-digit millisecond granularity.🛠 Proposed fix
- "log_time_format": "%Y-%m-%d %H:%M:%S.%f", + "log_time_format": "%Y-%m-%d %H:%M:%S",Rich's
RichHandlerrenders its own millisecond column separately whenshow_time=True, so the sub-second portion inlog_time_formatis typically redundant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log.py` at line 90, The log_time_format currently uses "%Y-%m-%d %H:%M:%S.%f" which produces 6-digit microseconds instead of the intended 3-digit milliseconds; remove the "%f" sub-second directive (e.g., use "%Y-%m-%d %H:%M:%S") so RichHandler (with show_time=True) renders milliseconds itself and the non-Rich path continues to use %(msecs)03d from DEFAULT_LOG_FORMAT; update the log_time_format constant and ensure any RichHandler configuration does not rely on "%f".
73-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
setup_logging()is missing a docstring.The function lacks a docstring describing its behavior, side effects, and return value.
🛠 Proposed fix
`@lru_cache` def setup_logging() -> dict[t.Any, t.Any]: + """Configure and return the merged logging configuration. + + Selects a Rich or uvicorn default handler based on TTY detection and the + LIGHTSPEED_STACK_DISABLE_RICH_HANDLER environment variable, merges the + computed configuration into uvicorn's default logging config, applies it + via logging.config.dictConfig, and returns the merged configuration dict. + Result is cached; call setup_logging.cache_clear() to force re-initialization. + + Returns: + dict: The merged logging configuration passed to dictConfig. + """As per coding guidelines,
src/**/*.pyrequires "All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log.py` around lines 73 - 74, Add a descriptive docstring to setup_logging() that explains what the function does (configures/initializes logging), any side effects (registers handlers, configures root logger, uses lru_cache to memoize), and what it returns (a dict of the logging configuration). Place the docstring immediately under the `@lru_cache-decorated` def setup_logging(...) and ensure it uses complete, clear language describing parameters (none), return value (dict[t.Any, t.Any] or updated to modern typing if you choose), and any important notes about caching/side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runners/uvicorn.py`:
- Around line 13-25: The docstring for start_uvicorn is missing documentation
for the new log_config parameter; update the function's docstring to add a
"Parameters" entry for log_config (type dict | None) describing that it supplies
an optional uvicorn logging configuration passed to uvicorn.run and that None
uses default logging behavior, and ensure the return type sentence remains
accurate for the function signature of start_uvicorn.
---
Duplicate comments:
In `@src/log.py`:
- Line 11: The import from pydantic.v1.utils using deep_update is relying on an
internal v1-compat API; remove the line importing deep_update and add a small
local helper named _deep_update that recursively merges two dicts (signature
_deep_update(base: dict, override: dict) -> dict), then replace all usages of
deep_update(...) in this module with _deep_update(...); ensure the helper
returns a new dict and performs recursive merging when both values are dicts to
preserve current behavior of deep_update.
- Around line 108-119: setup_logging currently calls
deep_update(uvicorn.config.LOGGING_CONFIG, logging_conf) then mutates
merged_config (and thus uvicorn.config.LOGGING_CONFIG) via assignments to
merged_config["formatters"] and merged_config["loggers"], corrupting global
state; fix by avoiding in-place mutation of uvicorn's global: either deepcopy
uvicorn.config.LOGGING_CONFIG before merging (use copy.deepcopy on
uvicorn.config.LOGGING_CONFIG into merged_config) or build all conditional keys
into logging_conf first (populate logging_conf["formatters"] and
logging_conf["loggers"] based on handler) and then call deep_update once; update
references to merged_config, deep_update, logging_conf and setup_logging
accordingly so no assignment mutates uvicorn.config.LOGGING_CONFIG.
- Line 90: The log_time_format currently uses "%Y-%m-%d %H:%M:%S.%f" which
produces 6-digit microseconds instead of the intended 3-digit milliseconds;
remove the "%f" sub-second directive (e.g., use "%Y-%m-%d %H:%M:%S") so
RichHandler (with show_time=True) renders milliseconds itself and the non-Rich
path continues to use %(msecs)03d from DEFAULT_LOG_FORMAT; update the
log_time_format constant and ensure any RichHandler configuration does not rely
on "%f".
- Around line 73-74: Add a descriptive docstring to setup_logging() that
explains what the function does (configures/initializes logging), any side
effects (registers handlers, configures root logger, uses lru_cache to memoize),
and what it returns (a dict of the logging configuration). Place the docstring
immediately under the `@lru_cache-decorated` def setup_logging(...) and ensure it
uses complete, clear language describing parameters (none), return value
(dict[t.Any, t.Any] or updated to modern typing if you choose), and any
important notes about caching/side effects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69e249b9-b492-46f9-bcfd-45f3a03de2c9
📒 Files selected for processing (5)
src/constants.pysrc/lightspeed_stack.pysrc/log.pysrc/runners/uvicorn.pytests/unit/test_log.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Pyright
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: unit_tests (3.12)
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: Pylinter
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/lightspeed_stack.pysrc/constants.pysrc/runners/uvicorn.pysrc/log.py
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
constants.pyfor shared constants with descriptive comments and type hints usingFinal[type]
Files:
src/constants.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/test_log.py
🔇 Additional comments (2)
src/constants.py (1)
228-228: ⚡ Quick win
DEFAULT_LOGGER_NAMEis missing itsFinal[str]annotation.Every other constant in this file uses
Final[type]. This one does not, leaving it open to accidental reassignment.🛠 Proposed fix
-DEFAULT_LOGGER_NAME = "lightspeed_stack" +DEFAULT_LOGGER_NAME: Final[str] = "lightspeed_stack"As per coding guidelines,
src/constants.pyrequires "type hints usingFinal[type]" for shared constants.src/log.py (1)
1-1: No Black formatting issues found.The file complies with Black formatting standards. All lines are under 88 characters, imports are properly organized (stdlib, third-party, local), and spacing between top-level functions follows Black conventions (two blank lines). The file is ready for merge from a formatting perspective.
samdoran
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/log.py (3)
13-13: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
pydantic.v1.utils.deep_updateis an internal v1-compat shim — replace with a self-contained helperThis import uses an internal API from pydantic's v1 compatibility layer, creating unnecessary coupling for a simple utility function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log.py` at line 13, Replace the internal import of pydantic.v1.utils.deep_update with a local helper: remove the line importing deep_update and add a small self-contained deep_update function in src/log.py that recursively merges dict-like structures (preserving nested dicts and replacing non-dict values), then update all usages in this file to call the new local deep_update helper (referenced by its function name deep_update) so the module no longer depends on pydantic internals.
82-83: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExpand
setup_logging()docstring to document side effects and caching.The current docstring is too brief. It should explain that this function configures logging globally via
dictConfig, is cached vialru_cache, and describe the returned merged configuration structure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log.py` around lines 82 - 83, Update the setup_logging() docstring to describe side effects and caching: state that setup_logging() globally configures Python logging by calling dictConfig (applies side effects), is decorated with lru_cache so repeated calls return the cached configuration, and return value is the merged dict configuration (handlers, formatters, loggers, level info) that was applied; mention any notable keys in the returned structure and that callers receive the applied config rather than a fresh copy.
119-128: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider moving post-merge assignments into
logging_confbefore thedeep_updatecall.While
deepcopyprevents mutating the global uvicorn config, constructing all conditional configuration insidelogging_confbefore callingdeep_updatewould eliminate post-merge mutations entirely and make the merge operation clearer.♻️ Proposed refactor
+ # Build conditional formatters/loggers before merge if handler == "rich": - merged_config = deep_update(deepcopy(uvicorn.config.LOGGING_CONFIG), logging_conf) - merged_config["loggers"]["uvicorn"]["handlers"] = [handler] - merged_config["loggers"]["uvicorn.access"]["handlers"] = [handler] + logging_conf["loggers"]["uvicorn"] = {"handlers": ["rich"]} + logging_conf["loggers"]["uvicorn.access"] = {"handlers": ["rich"]} else: - merged_config = deep_update(deepcopy(uvicorn.config.LOGGING_CONFIG), logging_conf) - merged_config["formatters"]["access"]["fmt"] = ( - "%(asctime)s.%(msecs)03d %(levelprefix)s " - '%(client_addr)s - "%(request_line)s" %(status_code)s' - ) - merged_config["formatters"]["default"]["fmt"] = DEFAULT_LOG_FORMAT - merged_config["formatters"]["default"]["datefmt"] = "%Y-%m-%d %H:%M" + logging_conf["formatters"] = { + "access": { + "fmt": "%(asctime)s.%(msecs)03d %(levelprefix)s " + '%(client_addr)s - "%(request_line)s" %(status_code)s', + }, + "default": { + "fmt": DEFAULT_LOG_FORMAT, + "datefmt": "%Y-%m-%d %H:%M", + }, + } + + merged_config = deep_update(deepcopy(uvicorn.config.LOGGING_CONFIG), logging_conf)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log.py` around lines 119 - 128, The post-merge mutations to merged_config (setting merged_config["loggers"]["uvicorn"]["handlers"], merged_config["loggers"]["uvicorn.access"]["handlers"], and the formatters under merged_config) should be moved into the logging_conf construction before calling deep_update so the deep_update call produces the final merged_config without later edits; update the logic that currently branches on handler == "rich" to instead set the corresponding keys on logging_conf (e.g., logging_conf["loggers"]["uvicorn"]["handlers"], logging_conf["loggers"]["uvicorn.access"]["handlers"] or logging_conf["formatters"]["access"]["fmt"], logging_conf["formatters"]["default"]["fmt"] and logging_conf["formatters"]["default"]["datefmt"] using DEFAULT_LOG_FORMAT) and then call deep_update(base, logging_conf) once to produce merged_config.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runners/uvicorn.py`:
- Around line 17-25: Fix the malformed docstring for the Uvicorn server starter
in src/runners/uvicorn.py: rewrite the Parameters section so each parameter has
its own complete description (e.g., configuration: ServiceConfiguration
providing host, port, workers and tls_config; log_config: dict for logging
configuration), ensure tls_config and its fields (`tls_key_path`,
`tls_certificate_path`, `tls_key_password`) are documented as possibly None, and
add a Returns section documenting that the function returns None; update the
summary and parameter order to follow Google Python docstring conventions.
---
Duplicate comments:
In `@src/log.py`:
- Line 13: Replace the internal import of pydantic.v1.utils.deep_update with a
local helper: remove the line importing deep_update and add a small
self-contained deep_update function in src/log.py that recursively merges
dict-like structures (preserving nested dicts and replacing non-dict values),
then update all usages in this file to call the new local deep_update helper
(referenced by its function name deep_update) so the module no longer depends on
pydantic internals.
- Around line 82-83: Update the setup_logging() docstring to describe side
effects and caching: state that setup_logging() globally configures Python
logging by calling dictConfig (applies side effects), is decorated with
lru_cache so repeated calls return the cached configuration, and return value is
the merged dict configuration (handlers, formatters, loggers, level info) that
was applied; mention any notable keys in the returned structure and that callers
receive the applied config rather than a fresh copy.
- Around line 119-128: The post-merge mutations to merged_config (setting
merged_config["loggers"]["uvicorn"]["handlers"],
merged_config["loggers"]["uvicorn.access"]["handlers"], and the formatters under
merged_config) should be moved into the logging_conf construction before calling
deep_update so the deep_update call produces the final merged_config without
later edits; update the logic that currently branches on handler == "rich" to
instead set the corresponding keys on logging_conf (e.g.,
logging_conf["loggers"]["uvicorn"]["handlers"],
logging_conf["loggers"]["uvicorn.access"]["handlers"] or
logging_conf["formatters"]["access"]["fmt"],
logging_conf["formatters"]["default"]["fmt"] and
logging_conf["formatters"]["default"]["datefmt"] using DEFAULT_LOG_FORMAT) and
then call deep_update(base, logging_conf) once to produce merged_config.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6d1f3f5-970f-406c-a46d-ace4906571a5
📒 Files selected for processing (3)
src/constants.pysrc/log.pysrc/runners/uvicorn.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: radon
- GitHub Check: mypy
- GitHub Check: Pylinter
- GitHub Check: spectral
- GitHub Check: integration_tests (3.13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: unit_tests (3.13)
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/runners/uvicorn.pysrc/constants.pysrc/log.py
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
constants.pyfor shared constants with descriptive comments and type hints usingFinal[type]
Files:
src/constants.py
🔇 Additional comments (3)
src/log.py (2)
25-27: LGTM — millisecond formatting helper is well-implemented.The helper correctly converts microseconds to 3-digit milliseconds and returns Rich's
Texttype for use inlog_time_format.
98-98: ⚡ Quick winThe code correctly uses a callable for
log_time_format.The Rich library's
RichHandlerexplicitly acceptsUnion[str, FormatTimeCallable]for thelog_time_formatparameter, so passing_ms_time_format(a callable) is fully supported and correct.src/runners/uvicorn.py (1)
29-30: LGTM — correct fallback to unified logging configuration.The conditional call to
setup_logging()properly applies the default merged configuration whenlog_configis not provided.
samdoran
force-pushed
the
use-existing-logger
branch
from
1b0f4ac to
e0e20f1
Compare
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
please try to figure out why unit tests fail. LGTM in overall
|
Yup, I'm working on the unit test failures. |
samdoran
force-pushed
the
use-existing-logger
branch
from
e0e20f1 to
d07f50b
Compare
|
The problem with the tests was |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/log.py (2)
13-13: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReplace
pydantic.v1.utils.deep_updatewith a self-contained helper.
pydantic.v1.utilsis part of the v1 compatibility layer, not a stable public API. Relying on internal utilities creates an unnecessary coupling for a simple recursive merge.♻️ Proposed replacement
Remove the pydantic import:
-from pydantic.v1.utils import deep_updateAdd a local helper function after the imports:
def _deep_update(base: dict, override: dict) -> dict: """Recursively merge override into base, returning a new dict.""" result = base.copy() for key, value in override.items(): if key in result and isinstance(result[key], dict) and isinstance(value, dict): result[key] = _deep_update(result[key], value) else: result[key] = value return resultThen replace the call on line 117:
- merged_config = deep_update(deepcopy(uvicorn.config.LOGGING_CONFIG), logging_conf) + merged_config = _deep_update(deepcopy(uvicorn.config.LOGGING_CONFIG), logging_conf)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log.py` at line 13, Replace the fragile import of deep_update from pydantic.v1.utils with a small self-contained recursive merge helper: remove the import "from pydantic.v1.utils import deep_update", add a local function named _deep_update(base: dict, override: dict) that recursively merges override into base (creating and returning a new dict), and then update any call sites that reference deep_update (e.g., the call in the logging configuration merge where deep_update(...) is used) to call _deep_update(...) instead so the code no longer depends on pydantic's internal API.
81-83: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExpand docstring to follow Google-style conventions.
The coding guidelines require descriptive docstrings following Google Python docstring conventions with required sections including Returns. The current one-line docstring omits the Returns section and does not explain side effects or caching behavior.
📝 Proposed docstring
`@lru_cache` def setup_logging() -> dict[t.Any, t.Any]: - """Create logging configuration.""" + """Configure and return the merged logging configuration. + + Selects a Rich or uvicorn default handler based on TTY detection and the + LIGHTSPEED_STACK_DISABLE_RICH_HANDLER environment variable, merges the + computed configuration into uvicorn's default logging config, applies it + via logging.config.dictConfig, and returns the merged configuration dict. + + Result is cached; call setup_logging.cache_clear() to force re-initialization. + + Returns: + dict: The merged logging configuration passed to dictConfig. + """As per coding guidelines, all functions must include descriptive docstrings following Google Python docstring conventions with required sections: Parameters, Returns, Raises.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log.py` around lines 81 - 83, The setup_logging function has only a one-line docstring; update it to a Google-style docstring that documents that the function configures the global logging system (side effects), is cached via `@lru_cache` (so repeated calls return the same dict), lists Parameters: none (or omit if preferred), a Returns section describing the returned logging configuration dict (type: dict[Any, Any] or more specific mapping of handlers/formatters), and a Raises section (e.g., none expected or list specific exceptions if applicable); reference the setup_logging function and its `@lru_cache` decorator when editing the docstring so it clearly states caching behavior and side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/app/endpoints/test_rlsapi_v1.py`:
- Around line 791-806: The logger name construction in the caplog.at_level call
uses a double-dot: f"{constants.DEFAULT_LOGGER_NAME}..app.endpoints.rlsapi_v1"
which produces a malformed name; update that expression to use a single-dot and
guard against a trailing dot on DEFAULT_LOGGER_NAME—for example replace with
f"{constants.DEFAULT_LOGGER_NAME.rstrip('.')}.app.endpoints.rlsapi_v1" (change
in the test that calls caplog.at_level, referencing
constants.DEFAULT_LOGGER_NAME and the logger string).
- Around line 709-726: The test constructs a malformed logger name with a
double-dot in caplog.at_level; change the logger argument from
f"{constants.DEFAULT_LOGGER_NAME}..app.endpoints.rlsapi_v1" to use a single dot
(e.g., f"{constants.DEFAULT_LOGGER_NAME}.app.endpoints.rlsapi_v1") so caplog
attaches to the correct logger; update the string in the test around the
caplog.at_level call (referencing constants.DEFAULT_LOGGER_NAME and the
caplog.at_level invocation) to remove the extra dot.
In `@tests/unit/authorization/test_azure_token_manager.py`:
- Around line 140-147: The logger name in the test uses a malformed double-dot:
change the f-string constructing the logger in the caplog.at_level call from
f"{DEFAULT_LOGGER_NAME}..authorization.azure_token_manager" to use a single dot
(e.g., f"{DEFAULT_LOGGER_NAME}.authorization.azure_token_manager") so the logger
hierarchy is correct; update the caplog context around
token_manager.refresh_token() accordingly to use DEFAULT_LOGGER_NAME with a
single dot.
In `@tests/unit/models/config/test_rlsapi_v1_configuration.py`:
- Around line 164-169: The caplog.at_level call builds a malformed logger name
using a double-dot string; change the logger argument to use a single-dot
separator by replacing f"{DEFAULT_LOGGER_NAME}..models.config" with
f"{DEFAULT_LOGGER_NAME}.models.config" so the test uses a correct hierarchical
logger name (affecting the caplog context around Configuration(**config_dict)
and the subsequent assertion).
- Around line 137-142: The logger name in the test uses a malformed double-dot;
update the logger string in the caplog.at_level call to use a single dot (change
f"{DEFAULT_LOGGER_NAME}..models.config" to
f"{DEFAULT_LOGGER_NAME}.models.config") so the logger hierarchy is correct;
locate the caplog.at_level invocation in the test (and the DEFAULT_LOGGER_NAME
reference) and adjust the string accordingly.
In `@tests/unit/runners/test_uvicorn_runner.py`:
- Line 23: Add a unit test that covers the default log_config=None path by
calling start_uvicorn(configuration) without passing log_config, patching
runners.uvicorn.setup_logging to return a controlled dict and asserting it was
called, and patching uvicorn.run to assert its log_config parameter equals the
mocked return value; reference start_uvicorn (the function under test),
setup_logging (to be mocked), and uvicorn.run (to assert call) when implementing
the test in tests/unit/runners/test_uvicorn_runner.py.
---
Duplicate comments:
In `@src/log.py`:
- Line 13: Replace the fragile import of deep_update from pydantic.v1.utils with
a small self-contained recursive merge helper: remove the import "from
pydantic.v1.utils import deep_update", add a local function named
_deep_update(base: dict, override: dict) that recursively merges override into
base (creating and returning a new dict), and then update any call sites that
reference deep_update (e.g., the call in the logging configuration merge where
deep_update(...) is used) to call _deep_update(...) instead so the code no
longer depends on pydantic's internal API.
- Around line 81-83: The setup_logging function has only a one-line docstring;
update it to a Google-style docstring that documents that the function
configures the global logging system (side effects), is cached via `@lru_cache`
(so repeated calls return the same dict), lists Parameters: none (or omit if
preferred), a Returns section describing the returned logging configuration dict
(type: dict[Any, Any] or more specific mapping of handlers/formatters), and a
Raises section (e.g., none expected or list specific exceptions if applicable);
reference the setup_logging function and its `@lru_cache` decorator when editing
the docstring so it clearly states caching behavior and side effects.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48a93a80-1deb-4ae1-bffc-dafd39deef27
📒 Files selected for processing (10)
src/constants.pysrc/lightspeed_stack.pysrc/log.pysrc/runners/uvicorn.pytests/unit/app/endpoints/test_rlsapi_v1.pytests/unit/authorization/test_azure_token_manager.pytests/unit/conftest.pytests/unit/models/config/test_rlsapi_v1_configuration.pytests/unit/runners/test_uvicorn_runner.pytests/unit/test_log.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: shellcheck
- GitHub Check: black
- GitHub Check: Pyright
- GitHub Check: Pylinter
- GitHub Check: unit_tests (3.13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: library mode / ci / group 1
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/lightspeed_stack.pysrc/runners/uvicorn.pysrc/constants.pysrc/log.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/conftest.pytests/unit/authorization/test_azure_token_manager.pytests/unit/runners/test_uvicorn_runner.pytests/unit/models/config/test_rlsapi_v1_configuration.pytests/unit/app/endpoints/test_rlsapi_v1.pytests/unit/test_log.py
tests/**/conftest.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
conftest.pyfor shared pytest fixtures andpytest-mockfor AsyncMock objects
Files:
tests/unit/conftest.py
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
constants.pyfor shared constants with descriptive comments and type hints usingFinal[type]
Files:
src/constants.py
🔇 Additional comments (9)
src/constants.py (1)
228-233: LGTM!tests/unit/conftest.py (1)
25-50: LGTM!src/lightspeed_stack.py (1)
13-19: LGTM!tests/unit/test_log.py (1)
14-74: LGTM!Also applies to: 77-115
src/runners/uvicorn.py (5)
17-26: Fix the malformed docstring.The docstring violates Google Python style conventions. This issue was already identified in a previous review.
7-7: LGTM!
13-16: LGTM!
39-39: LGTM!
29-30: The return type ofsetup_logging()is correctly annotated asdict[t.Any, t.Any]and is fully compatible withuvicorn.run(log_config=...).The function merges uvicorn's built-in
LOGGING_CONFIGwith custom logging settings, ensuring the returned dictionary conforms to Uvicorn's expected logging configuration schema. No changes needed.
samdoran
force-pushed
the
use-existing-logger
branch
2 times, most recently
from
ae1a1c9 to
4e8d95a
Compare
Meroge the existing config from uvicorn with the logging for lightspeed-stack with some modifications and pass that to uvicorn. This ensures the logging configs work together and do not clobber each other. Call setup_logging() early in the main entrypoint.
…y in __name__. The logging library assumes __name__ will be “package.module.module”. Since this project does not have a package, the value for __name__ varies widely in each module. Thise breaks design assumpmtions of logging. To work around this, define a default logger name that is used as the primary configuration and add a helper function to always get the logger with a name that aligns with how logging works.
Since there is no root package for this project, manually set the logger and get the filename in order to show where the log message was emitted.
Use levelprefix for uvicorn.logging.DefaultFormatte. Move filename and position to end of line so that information is arranged in most important order from left to right within the line, where the message came from being least relevant in my thinking compared to the time, log level, and actual message.
When rich is not selected, use the uvicorn.logging.DefaultFormatter for log messages. Modify the default format slightly to include miliseconds in the timestamp.
Add a description of the problem to be addressed in the future.
The default .%f handling in RichHandler gives microseconds and strftime does not provide a milisecond format string.
samdoran
force-pushed
the
use-existing-logger
branch
from
4e8d95a to
c0ac8e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/unit/runners/test_uvicorn_runner.py (1)
194-204:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the successful default-log-config flow, not only error propagation.
Line 194 currently proves
setup_logging()is invoked by forcing it to raise, but it does not verify that its returned config is passed touvicorn.run(log_config=...)in the normal path.🔧 Proposed test adjustment
def test_start_uvicorn_no_log_config(mocker: MockerFixture) -> None: """Test that the default logging config is used when none is provided.""" configuration = ServiceConfiguration( host="localhost", port=8080, workers=1 ) # pyright: ignore[reportCallIssue] - mock_setup_logging = mocker.patch("runners.uvicorn.setup_logging") - mock_setup_logging.side_effect = ValueError("Raised intentionally") - - with pytest.raises(ValueError, match="Raised intentionally"): - start_uvicorn(configuration) + mock_log_config = {"version": 1, "formatters": {}} + mock_setup_logging = mocker.patch( + "runners.uvicorn.setup_logging", return_value=mock_log_config + ) + mocked_run = mocker.patch("uvicorn.run") + + start_uvicorn(configuration) + + mock_setup_logging.assert_called_once_with() + mocked_run.assert_called_once() + assert mocked_run.call_args.kwargs["log_config"] == mock_log_config🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/runners/test_uvicorn_runner.py` around lines 194 - 204, Update the test_start_uvicorn_no_log_config to assert the normal/default logging flow instead of only error propagation: patch runners.uvicorn.setup_logging to return a fake logging dict (e.g., fake_log_config), patch uvicorn.run to a mock, call start_uvicorn(ServiceConfiguration(...)), and then assert uvicorn.run was called once with log_config equal to the fake_log_config (and other expected args), ensuring start_uvicorn forwards the returned config from setup_logging to uvicorn.run; reference the functions start_uvicorn, setup_logging and uvicorn.run to locate the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lightspeed_stack.py`:
- Line 18: The call to setup_logging() at import time causes handlers to be
created with preset levels before CLI args (like --verbose) can set
LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR, so move the setup_logging() invocation out
of module-level import and invoke it after argument parsing (or after you set
LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR) — locate the setup_logging() call in
src/lightspeed_stack.py and remove it from top-level execution, then call
setup_logging() from the entrypoint function that handles CLI args (or
re-run/clear logging configuration after setting
LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR) so handlers respect the effective log level.
In `@src/log.py`:
- Line 13: The import of pydantic.v1.utils.deep_update in src/log.py is a v1
compatibility shim; replace it by implementing and using a small local
deep-merge helper (e.g., merge_dicts or deep_merge) or a stdlib-safe alternative
and update all usages to call that helper instead of deep_update; locate the
import line and any call sites that reference deep_update and swap them to use
the new function (name the helper uniquely like deep_merge_dicts so it's easy to
find and test), ensure the helper correctly merges nested dicts and list values
the same way deep_update was expected to so logging configuration behavior
remains unchanged.
In `@src/runners/uvicorn.py`:
- Around line 25-26: The docstring for the function that accepts the parameter
log_config must be updated to match the signature: change the parameter type to
Optional[dict] (i.e., dict | None) and document that it is optional in the
Parameters section; add a Returns section that documents the actual return value
of the function (e.g., None or the process/object returned by the runner) and
include any exceptions in a Raises section if the function can raise errors.
Locate the function that defines the log_config parameter (search for log_config
in src/runners/uvicorn.py) and update its Google-style docstring (Parameters,
Returns, Raises) to reflect the current API.
---
Duplicate comments:
In `@tests/unit/runners/test_uvicorn_runner.py`:
- Around line 194-204: Update the test_start_uvicorn_no_log_config to assert the
normal/default logging flow instead of only error propagation: patch
runners.uvicorn.setup_logging to return a fake logging dict (e.g.,
fake_log_config), patch uvicorn.run to a mock, call
start_uvicorn(ServiceConfiguration(...)), and then assert uvicorn.run was called
once with log_config equal to the fake_log_config (and other expected args),
ensuring start_uvicorn forwards the returned config from setup_logging to
uvicorn.run; reference the functions start_uvicorn, setup_logging and
uvicorn.run to locate the code under test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0fef9b9-93fe-426b-9353-9478636e19fb
📒 Files selected for processing (10)
src/constants.pysrc/lightspeed_stack.pysrc/log.pysrc/runners/uvicorn.pytests/unit/app/endpoints/test_rlsapi_v1.pytests/unit/authorization/test_azure_token_manager.pytests/unit/conftest.pytests/unit/models/config/test_rlsapi_v1_configuration.pytests/unit/runners/test_uvicorn_runner.pytests/unit/test_log.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: unit_tests (3.12)
- GitHub Check: Pylinter
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/models/config/test_rlsapi_v1_configuration.pytests/unit/conftest.pytests/unit/authorization/test_azure_token_manager.pytests/unit/test_log.pytests/unit/runners/test_uvicorn_runner.pytests/unit/app/endpoints/test_rlsapi_v1.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/lightspeed_stack.pysrc/runners/uvicorn.pysrc/constants.pysrc/log.py
tests/**/conftest.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
conftest.pyfor shared pytest fixtures andpytest-mockfor AsyncMock objects
Files:
tests/unit/conftest.py
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
constants.pyfor shared constants with descriptive comments and type hints usingFinal[type]
Files:
src/constants.py
🔇 Additional comments (7)
tests/unit/app/endpoints/test_rlsapi_v1.py (1)
709-711: Logger name still contains a malformed double-dot separator.Both
caplog.at_level(...)logger strings still use..and should use a single.hierarchy separator.Also applies to: 791-794
src/constants.py (1)
233-237: LGTM!tests/unit/authorization/test_azure_token_manager.py (1)
140-147: LGTM!tests/unit/conftest.py (1)
25-50: LGTM!tests/unit/models/config/test_rlsapi_v1_configuration.py (1)
137-142: LGTM!Also applies to: 164-169
tests/unit/runners/test_uvicorn_runner.py (1)
23-23: LGTM!Also applies to: 35-35, 47-47, 59-59, 72-72, 84-84, 101-101, 113-113, 125-125, 137-137, 178-178, 190-190
tests/unit/test_log.py (1)
8-8: LGTM!Also applies to: 11-11, 16-22, 29-32, 61-64, 71-74, 95-95, 104-104, 113-113, 115-115
| force=True, | ||
| ) | ||
|
|
||
| setup_logging() |
There was a problem hiding this comment.
setup_logging() runs too early and can neutralize --verbose.
Calling setup_logging() at import time sets handler levels before CLI args are parsed. If --verbose is passed later, logger levels change to DEBUG but handlers may still filter at INFO. Move setup until after argument parsing (or clear cache and rerun after setting LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lightspeed_stack.py` at line 18, The call to setup_logging() at import
time causes handlers to be created with preset levels before CLI args (like
--verbose) can set LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR, so move the
setup_logging() invocation out of module-level import and invoke it after
argument parsing (or after you set LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR) — locate
the setup_logging() call in src/lightspeed_stack.py and remove it from top-level
execution, then call setup_logging() from the entrypoint function that handles
CLI args (or re-run/clear logging configuration after setting
LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR) so handlers respect the effective log level.
|
|
||
| from rich.logging import RichHandler | ||
| import uvicorn.config | ||
| from pydantic.v1.utils import deep_update |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is pydantic.v1.utils.deep_update considered a stable public API in Pydantic v2, or a v1-compat/internal shim that may be removed in v3?
💡 Result:
No: in Pydantic v2, pydantic.v1.utils.deep_update is not presented as a stable public API for v2-era users; it is part of the Pydantic v1 compatibility surface (pydantic.v1), and is therefore effectively a v1-compat/internal shim that may be removed in a future major version (v3). Evidence: - The Pydantic v2 docs explicitly state that the v2 package continues to provide access to the Pydantic V1 API via the pydantic.v1 namespace for migration/compatibility purposes. That framing implies these items are maintained for compatibility rather than being a stable v2 API surface.[1] - The migration guide maps pydantic.utils.deep_update (v1) to pydantic.v1.utils.deep_update, indicating deep_update lives under the v1 namespace when using v2.[1] - A StackOverflow answer reproduces the runtime deprecation/removal behavior of pydantic.utils:deep_update, showing that importing from the top-level v1 location is no longer the supported route in v2; users are directed to pydantic.v1.utils.deep_update instead.[2] - Concretely, the code itself still exists as deep_update in the v1 utils.py implementation, but this does not by itself imply stability for v2; it supports the conclusion that it’s retained specifically for v1 API compatibility.[3] So, if you’re writing new code targeting Pydantic v2, treat pydantic.v1.utils.deep_update as legacy/compatibility rather than a stable v2 public API; if you need long-term stability, prefer v2-native utilities (or vendor your own small deep-merge helper) rather than depending on the v1-compat function being present in v3.[1][2]
Citations:
- 1: https://pydantic.dev/docs/validation/latest/get-started/migration/
- 2: https://stackoverflow.com/questions/3232943/update-value-of-a-nested-dictionary-of-varying-depth
- 3: https://github.com/pydantic/pydantic/blob/1.10.X-fixes/pydantic/utils.py
Replace pydantic.v1.utils.deep_update with a stable alternative to avoid v1 compatibility dependencies.
pydantic.v1.utils.deep_update is a v1 compatibility shim, not a stable v2 public API, and may be removed in future Pydantic versions. Replace it with a local deep-merge helper or a standard library utility to keep the logging setup independent of v1 compatibility layers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/log.py` at line 13, The import of pydantic.v1.utils.deep_update in
src/log.py is a v1 compatibility shim; replace it by implementing and using a
small local deep-merge helper (e.g., merge_dicts or deep_merge) or a stdlib-safe
alternative and update all usages to call that helper instead of deep_update;
locate the import line and any call sites that reference deep_update and swap
them to use the new function (name the helper uniquely like deep_merge_dicts so
it's easy to find and test), ensure the helper correctly merges nested dicts and
list values the same way deep_update was expected to so logging configuration
behavior remains unchanged.
| log_config (dict): Logging configuration. | ||
| """ |
There was a problem hiding this comment.
Docstring is out of sync with signature and missing Returns.
log_config is optional (dict | None) in code but documented as dict, and the function docstring lacks a Returns section. Update the docstring to match the current API.
As per coding guidelines, functions must follow “Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runners/uvicorn.py` around lines 25 - 26, The docstring for the function
that accepts the parameter log_config must be updated to match the signature:
change the parameter type to Optional[dict] (i.e., dict | None) and document
that it is optional in the Parameters section; add a Returns section that
documents the actual return value of the function (e.g., None or the
process/object returned by the runner) and include any exceptions in a Raises
section if the function can raise errors. Locate the function that defines the
log_config parameter (search for log_config in src/runners/uvicorn.py) and
update its Google-style docstring (Parameters, Returns, Raises) to reflect the
current API.
2 participants
Description
Major overhaul of the logging system:
setup_logging()early on so that logging is setup once and cache the result.lightspeed_stackso that the logging configuration is applied correctly.The end result of these changes is that calling
get_logger(__name__)results in a logger instance wherelogger.info()and other methods display log messages as expected and formatted consistently.Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
None
Checklist before requesting a review
Testing
To run unit tests:
pytest tests/unit/test_log.pyTo test logging configuration without Rich logging:
LIGHTSPEED_STACK_DISABLE_RICH_HANDLER=1 lightspeed-stackorpython src/lightspeed_stack.pyTo test logging configuration using the default format string:
lightspeed-stackorpython src/lightspeed_stack.pySummary by CodeRabbit
Refactor
Tests