Fix llama_stack_client log levels #205
Conversation
Log levels on the llama_stack_client are currently broken and setting `LLAMA_STACK_LOG=debug` doesn't work as it should. Log levels are automatically changed to WARNING. This is caused by us blindly loading the `llama_stack` library, which initializes logging to WARNING except a subset of modules that can be modified using the `LLAMA_STACK_LOGGING` env var. With this patch we only import `llama_stack` when we are actually going to use it, so that when we use the normal llama-stack client we can set its logging level.
WalkthroughThe import statements for Changes
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/client.py (1)
57-72: Apply the same clean-ups to the async pathMirror the boolean simplification and folded import for consistency and to satisfy linters.
- if llama_stack_config.use_as_library_client is True: + if llama_stack_config.use_as_library_client: ... - from llama_stack.distribution.library_client import AsyncLlamaStackAsLibraryClient # type: ignore +# pylint: disable=import-outside-toplevel + from llama_stack.distribution.library_client import ( + AsyncLlamaStackAsLibraryClient, + ) # type: ignore
🧹 Nitpick comments (1)
src/client.py (1)
20-34: Avoid verbose boolean comparison
is Trueis unnecessary and triggerspylint’ssingleton-comparison. Rely on the value’s truthiness instead.- if llama_stack_config.use_as_library_client is True: + if llama_stack_config.use_as_library_client:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Black
src/client.py
[error] 1-1: Black formatting check failed. Run 'black --write' to fix code style issues.
🪛 GitHub Actions: Python linter
src/client.py
[warning] 25-25: pylint: Line too long (109/100) (line-too-long)
[warning] 62-62: pylint: Line too long (114/100) (line-too-long)
[warning] 25-25: pylint: Import outside toplevel (llama_stack.distribution.library_client.LlamaStackAsLibraryClient) (import-outside-toplevel)
[warning] 62-62: pylint: Import outside toplevel (llama_stack.distribution.library_client.AsyncLlamaStackAsLibraryClient) (import-outside-toplevel)
⏰ 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). (1)
- GitHub Check: build-pr
🔇 Additional comments (1)
src/client.py (1)
1-85: CI is red – run BlackGitHub Actions reports a Black failure. Execute
black src/client.py(orblack .) and repush to unblock the pipeline.
| logger.info("Using Llama stack as library client") | ||
| from llama_stack.distribution.library_client import LlamaStackAsLibraryClient # type: ignore | ||
| client = LlamaStackAsLibraryClient( | ||
| llama_stack_config.library_client_config_path | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fold long import and document the deliberate late import
The inner import fixes the logging bug, but the line exceeds 100 chars and still raises import-outside-toplevel. Break it and add an explicit pragma.
- from llama_stack.distribution.library_client import LlamaStackAsLibraryClient # type: ignore
+# pylint: disable=import-outside-toplevel
+ from llama_stack.distribution.library_client import (
+ LlamaStackAsLibraryClient,
+ ) # type: ignore📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info("Using Llama stack as library client") | |
| from llama_stack.distribution.library_client import LlamaStackAsLibraryClient # type: ignore | |
| client = LlamaStackAsLibraryClient( | |
| llama_stack_config.library_client_config_path | |
| ) | |
| logger.info("Using Llama stack as library client") | |
| # pylint: disable=import-outside-toplevel | |
| from llama_stack.distribution.library_client import ( | |
| LlamaStackAsLibraryClient, | |
| ) # type: ignore | |
| client = LlamaStackAsLibraryClient( | |
| llama_stack_config.library_client_config_path | |
| ) |
🧰 Tools
🪛 GitHub Actions: Python linter
[warning] 25-25: pylint: Line too long (109/100) (line-too-long)
[warning] 25-25: pylint: Import outside toplevel (llama_stack.distribution.library_client.LlamaStackAsLibraryClient) (import-outside-toplevel)
🤖 Prompt for AI Agents
In src/client.py around lines 24 to 28, the import statement inside the function
is too long and triggers an import-outside-toplevel warning. Break the import
into multiple lines to keep it under 100 characters and add a comment with a
pragma to explicitly ignore the import-outside-toplevel warning, documenting
that the late import is deliberate to fix the logging bug.
|
LGTM! 👍 As already stated above, linters are complaining. |
|
@Akrog r u going to take a look at this ? I think we can merge it once the linters issues are fixed. |
|
@Akrog Hello, is this one still relevant? |
4 participants
Description
Log levels on the llama_stack_client are currently broken and setting
LLAMA_STACK_LOG=debugdoesn't work as it should. Log levels areautomatically changed to WARNING.
This is caused by us blindly loading the
llama_stacklibrary, whichinitializes logging to WARNING except a subset of modules that can be
modified using the
LLAMA_STACK_LOGGINGenv var.With this patch we only import
llama_stackwhen we are actually goingto use it, so that when we use the normal llama-stack client we can set
its logging level.
Type of change
Checklist before requesting a review
Testing
Just run lightspeed-stack with a remote llama-stack and set the
LLAMA_STACK_LOGGINGtodebugand see that you don't get debug logs, but you do once you apply this patch.Summary by CodeRabbit