Repository navigation
Conversation
WalkthroughLLM model initialization now avoids overwriting existing model entries when config omits an Changes
Sequence Diagram(s)sequenceDiagram
participant Init as Model Init
participant File as YAML File
participant Config as Config
participant Registry as Model Registry
Init->>File: Read model list / lookup model
File-->>Init: entryFound / notFound
alt entryFound AND no api_key in Config
Init->>Registry: Keep existing entry (no override)
Registry-->>Init: existing entry retained
else entryMissing OR api_key provided in Config
Init->>Config: Load model values from config
Config-->>Init: model config (model, name, base_url, api_key, ...)
Init->>Registry: Create or update entry with config values
Registry-->>Init: entry created/updated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 2
🧹 Nitpick comments (1)
tests/core/test_llm_model_registry.py (1)
22-27: Consider using pytest's tmp_path fixture for automatic cleanup.The current implementation creates temp files with
delete=Falsebut never cleans them up, which can lead to disk space accumulation over many test runs.Consider refactoring to use pytest's built-in
tmp_pathfixture:- def create_temp_model_file(self, model_data: dict) -> str: + def create_temp_model_file(self, model_data: dict, tmp_path) -> str: """Create a temporary model list file for testing.""" - temp_file = tempfile.NamedTemporaryFile(mode='w', suffix='.yaml', delete=False) - yaml.dump(model_data, temp_file) - temp_file.flush() - return temp_file.name + temp_file = tmp_path / "model_list.yaml" + temp_file.write_text(yaml.dump(model_data)) + return str(temp_file)Then update each test method signature to accept
tmp_pathand pass it to the helper:def test_init_models_existing_model_with_api_key_overwrites(self, tmp_path): # ... temp_file = self.create_temp_model_file(model_data, tmp_path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/core/llm.py(3 hunks)tests/core/test_llm_model_registry.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
tests/core/test_llm_model_registry.pyholmes/core/llm.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers/tags
Files:
tests/core/test_llm_model_registry.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should mirror the source structure under tests/
Files:
tests/core/test_llm_model_registry.py
🧠 Learnings (1)
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
Repo: robusta-dev/holmesgpt PR: 610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Applied to files:
tests/core/test_llm_model_registry.pyholmes/core/llm.py
🧬 Code graph analysis (1)
tests/core/test_llm_model_registry.py (2)
holmes/config.py (1)
Config(46-521)holmes/core/llm.py (3)
LLMModelRegistry(520-716)ModelEntry(65-86)models(672-673)
⏰ 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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: build
🔇 Additional comments (2)
holmes/core/llm.py (1)
9-9: LGTM - imports are properly used.Both imports are used in the file and correctly placed at the top per coding guidelines.
Also applies to: 21-21
tests/core/test_llm_model_registry.py (1)
29-168: Excellent test coverage of the new conditional loading logic.The four test methods comprehensively validate all scenarios:
- Overwriting existing models when
api_keyis provided- Preserving existing models when
api_keyis not provided- Creating new model entries for non-existent models (both with and without
api_key)The tests correctly use
SecretStrfor API key comparisons and properly mock dependencies.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/core/test_llm_model_registry.py (2)
28-61: LGTM!Test correctly verifies that when
api_keyis provided in config, the existing model entry is completely overwritten. The assertions properly validate all fields including the SecretStr type for api_key.Per coding guidelines, consider adding return type annotation:
- def test_init_models_existing_model_with_api_key_overwrites(self): + def test_init_models_existing_model_with_api_key_overwrites(self) -> None:
63-157: Excellent test coverage!These three test methods comprehensively cover the critical scenarios:
- Preserve behavior: When an existing model entry exists and no
api_keyis provided, the original entry is correctly preserved (including checkingapi_basefrom YAML rather thanbase_url).- Create with api_key: New model entries are created when the model doesn't exist in the file, with all fields properly set.
- Create without api_key: New model entries are created even without an
api_keywhen the model doesn't exist, correctly validating thatapi_keyis None.The test logic aligns perfectly with the
_init_modelsbehavior:if not existing_model or self.config.api_key:.Per coding guidelines, consider adding return type annotations to these test methods:
- def test_init_models_existing_model_without_api_key_preserves(self): + def test_init_models_existing_model_without_api_key_preserves(self) -> None:- def test_init_models_nonexistent_model_creates_new_entry(self): + def test_init_models_nonexistent_model_creates_new_entry(self) -> None:- def test_init_models_nonexistent_model_without_api_key_still_creates(self): + def test_init_models_nonexistent_model_without_api_key_still_creates(self) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/core/test_llm_model_registry.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
tests/core/test_llm_model_registry.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers/tags
Files:
tests/core/test_llm_model_registry.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should mirror the source structure under tests/
Files:
tests/core/test_llm_model_registry.py
🧠 Learnings (1)
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
Repo: robusta-dev/holmesgpt PR: 610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Applied to files:
tests/core/test_llm_model_registry.py
🧬 Code graph analysis (1)
tests/core/test_llm_model_registry.py (2)
holmes/config.py (1)
Config(46-521)holmes/core/llm.py (2)
LLMModelRegistry(520-716)models(672-673)
⏰ 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). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: build
🔇 Additional comments (2)
tests/core/test_llm_model_registry.py (2)
1-9: LGTM!Imports are correctly placed at the top of the file and include all necessary modules for the test suite.
12-13: LGTM!Clear class definition with appropriate docstring describing test focus.
mainred
changed the title
| existing_model = self._llms.get(self.config.model) | ||
| # When self.config.api_key and self.config.model are both set, we assume the user wants to override the | ||
| # model entry from the model list file. | ||
| if not existing_model or self.config.api_key: |
There was a problem hiding this comment.
Thinking here loudly, I think that this is very confusing, in which cases the user wants to override the api key of the models.
Also, if he would like to override it he can't just set the value in the model list? we already have couple of places (toolset for example) that we are allowing those kind od override and it is complexing the code very much.
There was a problem hiding this comment.
I just want to make it safe when both model and api_key are specified in the configuration file(~/.homes/config), we pick the configuration file instead of from the model list.
I agree too much check makes the code complicated. I think we can allow the user either to specify the model config in either model list or configuration file, and in cli when model list is defined, he can specify which model to use through model flag.
There was a problem hiding this comment.
This merge the cli and server behavior when model list is defined.
There was a problem hiding this comment.
The thing that confuse me in that case is what happen if you set all the params like base_url in the model list and not in the config. It will override it?
I think it is very cumbersome. you either set all in the model list or set all in the config. In that case if the user set only the api key we will also overrider other params imlicitly and im not sure that this is the right behivour.
Are you sure that this is what we want to do?
There was a problem hiding this comment.
If the user set all in the model list, how will he select which model to use through CLI?
There was a problem hiding this comment.
Looks im missing something. In the suggested implementation we are overriding the model with the api_key, how this is connected to selecting the right model though cli?
@Avi-Robusta has an open pr that still need tests here: https://github.com/HolmesGPT/holmesgpt/pull/1096/files#diff-008177b02b41ee19dca44738e38891469986b5f51dfd58bc1b2ae22231471679R550-R556
In that pr you can select models from the model list and use them.
We still need to add tests + docs for this flow.
Can you help me understand what im missing in your suggested pr?
There was a problem hiding this comment.
The user scenario is the user specify the model list and select the model from CLL.
|
@moshemorad May I get a second review on this one? |
mainred
changed the title
|
Duplicate of #1096 |
2 participants
When the CLI user define model list, and specify the model flag, holmesgpt will be overwrite the existing model with empty ones if the other llm configurations are not present in the config.
We allow the user to specify the model only when there's no api_key defined the configuration file.
Test
The model_list.yaml contains
config.yaml contains
same case when I use
poetry run holmes ask "what pods are running?" --model=azure/gpt-4.1