LCORE- Unify logging configuration and setup#1703
Conversation
📝 WalkthroughWalkthroughThis 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 🚥 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)
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. |
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
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
from
c1f7bad to
fe7fb9b
Compare
samdoran
force-pushed
the
use-existing-logger
branch
from
ae1a1c9 to
4e8d95a
Compare
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