LCORE-391: Refactoring: proper Llama Stack config name #261
Conversation
WalkthroughAll occurrences of the class name 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: 0
🔭 Outside diff range comments (2)
src/client.py (2)
22-34:load()silently overwrites an already-initialised singletonThe refactor touched the type annotation, but the method will still happily create a new client every time it’s called.
Because the holder is a process-wide singleton, a double initialisation – especially from concurrent threads – can race and lead to unpredictable behaviour.def load(self, llama_stack_config: LlamaStackConfiguration) -> None: """Retrieve Llama stack client according to configuration.""" + if self._lsc is not None: + logger.debug("LlamaStackClient already initialised – skipping.") + returnAdding this cheap guard (and possibly a
threading.Lock) will protect against inadvertent re-initialisation.
58-70: Same re-initialisation / race issue for the async holderReplicate the early-exit guard in the async variant to keep both code paths consistent and safe:
async def load(self, llama_stack_config: LlamaStackConfiguration) -> None: """Retrieve Async Llama stack client according to configuration.""" + if self._lsc is not None: + logger.debug("AsyncLlamaStackClient already initialised – skipping.") + return
🧹 Nitpick comments (5)
tests/unit/test_client.py (1)
22-24: Fix typo in docstring (‘remove’ → ‘remote’).The docstring still says “remove client (server) mode”.
A tiny wording tweak keeps the test description clear.- """Test if Llama Stack can be initialized in remove client (server) mode.""" + """Test if Llama Stack can be initialized in remote client (server) mode."""src/models/config.py (1)
57-85: Update error-message strings to the new class spelling.With the class now called
LlamaStackConfiguration, the validator still emits messages starting with “LLama…”.
Keeping naming consistent avoids confusion when users see validation errors.- "LLama stack URL is not specified and library client mode is not specified" + "Llama stack URL is not specified and library client mode is not specified" @@ - "LLama stack URL is not specified and library client mode is not enabled" + "Llama stack URL is not specified and library client mode is not enabled" @@ - "LLama stack library client mode is enabled but a configuration file path is not specified" + "Llama stack library client mode is enabled but a configuration file path is not specified"tests/unit/models/test_config.py (1)
58-63: Synchronise docstring with new class name.Docstring still refers to
LLamaStackConfiguration.-"""Test the LLamaStackConfiguration constructor.""" +"""Test the LlamaStackConfiguration constructor."""src/client.py (2)
1-1: Doc-string still uses the old “LLama” spellingTiny nit: the header comment wasn’t updated, so the very first place a reader looks still contains the inconsistent capitalisation the refactor set out to remove.
-"""LLama stack client retrieval.""" +"""Llama stack client retrieval."""
12-12: Consider a relative import to avoid shadowing third-partymodelspackagesUsing the top-level
modelsimport can back-fire if the project is installed in an environment that already contains amodelspackage. A relative import is safer and makes the intent explicit:-from models.config import LlamaStackConfiguration +from .models.config import LlamaStackConfiguration(Adjust the leading dot to match the real package hierarchy.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/client.py(3 hunks)src/configuration.py(2 hunks)src/models/config.py(2 hunks)tests/unit/models/test_config.py(10 hunks)tests/unit/test_client.py(6 hunks)tests/unit/utils/test_common.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
tests/unit/utils/test_common.py (1)
src/models/config.py (1)
LlamaStackConfiguration(57-85)
tests/unit/test_client.py (1)
src/models/config.py (1)
LlamaStackConfiguration(57-85)
tests/unit/models/test_config.py (2)
src/models/config.py (1)
LlamaStackConfiguration(57-85)src/configuration.py (1)
llama_stack_configuration(63-68)
src/client.py (1)
src/models/config.py (1)
LlamaStackConfiguration(57-85)
src/configuration.py (1)
src/models/config.py (1)
LlamaStackConfiguration(57-85)
🔇 Additional comments (3)
tests/unit/test_client.py (1)
6-19: Rename looks consistent – no further issues.All imports and instantiations now reference
LlamaStackConfiguration; tests will import the right symbol.
No other functional changes – good work.Also applies to: 52-63, 67-75, 80-92
tests/unit/utils/test_common.py (1)
15-18: LGTM – rename applied cleanly.The updates are purely mechanical; all tests keep their intent and compile correctly.
Also applies to: 38-46, 80-88, 122-130, 170-178, 224-232, 268-276
src/configuration.py (1)
8-15: Import/property rename applied correctly – no action needed.Also applies to: 63-68
1 participant
Description
LCORE-391: Refactoring: proper Llama Stack config name
Type of change
Related Tickets & Documents
Summary by CodeRabbit
Bug Fixes
Tests