Repository navigation
fix: allow the user to config api key in config file - #1036
Conversation
WalkthroughReplaces dict-based LLM registry entries with a Pydantic ModelEntry, adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Config
participant Registry as LLMModelRegistry
participant DefaultLLM
App->>Config: _get_llm(model_key)
Config->>Registry: get_model_params(model_key)
Registry-->>Config: ModelEntry
Note over Config: extract fields from ModelEntry\nunwrap SecretStr api_key via get_secret_value()
Config->>DefaultLLM: __init__(model, api_key, api_base, api_version, base_url, custom_args, tracer, ...)
DefaultLLM-->>Config: llm_instance
Config-->>App: llm_instance
sequenceDiagram
autonumber
participant Registry as LLMModelRegistry
participant FS as Models YAML
participant ModelEntry
Registry->>FS: _parse_models_file(path)
FS-->>Registry: raw dict entries
Registry->>ModelEntry: ModelEntry.model_validate(entry_dict)
ModelEntry-->>Registry: ModelEntry
Note over Registry: store _llms as dict[string, ModelEntry]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
450bd8c to
b8b7eeb
Compare
![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
♻️ Duplicate comments (1)
holmes/core/llm.py (1)
45-46: Duplicate concern: name field redundancy.This TODO comment echoes the concern raised in the past review comment at this location. As noted previously, the
namefield appears unused except at line 493 and isn't utilized by DefaultLLM.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
holmes/config.py(3 hunks)holmes/core/llm.py(8 hunks)tests/config_class/test_config_api_base_version.py(2 hunks)tests/config_class/test_config_load_model_list.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:
holmes/core/llm.pytests/config_class/test_config_api_base_version.pytests/config_class/test_config_load_model_list.pyholmes/config.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/config_class/test_config_api_base_version.pytests/config_class/test_config_load_model_list.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should mirror the source structure under tests/
Files:
tests/config_class/test_config_api_base_version.pytests/config_class/test_config_load_model_list.py
🧬 Code graph analysis (4)
holmes/core/llm.py (3)
holmes/clients/robusta_client.py (2)
RobustaModelsResponse(17-23)fetch_robusta_models(27-42)holmes/utils/file_utils.py (1)
load_yaml_file(19-56)holmes/utils/env.py (1)
replace_env_vars_values(36-60)
tests/config_class/test_config_api_base_version.py (1)
holmes/config.py (1)
_get_llm(445-475)
tests/config_class/test_config_load_model_list.py (3)
tests/conftest.py (1)
server_config(33-56)holmes/config.py (2)
Config(46-481)llm_model_registry(126-129)holmes/core/llm.py (1)
models(551-552)
holmes/config.py (1)
holmes/core/llm.py (1)
get_model_params(519-545)
⏰ 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). (4)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: llm_evals
🔇 Additional comments (6)
holmes/core/llm.py (2)
416-416: LGTM: ModelEntry-based registry.The refactor from dict-based to ModelEntry-based storage is consistent throughout the registry. The type hints, return types, and validation logic align correctly.
Also applies to: 519-519, 551-551, 554-563
578-596: LGTM: Azure OpenAI parameter support.The updated signature and ModelEntry construction correctly support Azure-specific parameters (api_base, api_version) alongside the general api_key field.
tests/config_class/test_config_api_base_version.py (1)
60-84: LGTM: Comprehensive Azure parameter test.The test correctly verifies that Config._get_llm forwards api_key, api_base, and api_version to DefaultLLM for Azure OpenAI models. The assertions validate the positional arguments match the expected DefaultLLM signature.
holmes/config.py (2)
48-50: LGTM: Restores api_key configuration support.This addition addresses the regression introduced in PR #954, allowing users to configure api_key in the config file. The SecretStr type ensures secure handling, and the comment clarifies the fallback to environment variables.
447-448: LGTM: ModelEntry integration and SecretStr unwrapping.The refactor to retrieve model_entry and use model_dump(exclude_none=True) aligns with the ModelEntry-based registry changes. The SecretStr unwrapping at lines 460-461 correctly extracts the actual key value for LLM initialization.
Also applies to: 460-461
tests/config_class/test_config_load_model_list.py (1)
24-24: LGTM: Attribute-based access for ModelEntry.The migration from dict-style access (
.get("model"),["model"]) to attribute access (.model,.base_url,.name,.is_robusta_model) correctly reflects the ModelEntry refactor. The expected values remain unchanged, confirming test equivalence.Also applies to: 31-31, 38-41
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/llm.py (1)
548-549: Fix the type mismatch in get_llm.The method signature declares a return type of
LLM, butself._llms[name]returns aModelEntryinstance. This will cause type errors and runtime issues if the method is called.The TODO comment indicates awareness of this issue. Consider one of these solutions:
- Remove this method if it's unused
- Update the logic to construct an
LLMinstance from theModelEntry- Change the return type to
ModelEntryif callers expect model configuration rather than an instantiated LLMRun this script to check if the method is being called:
#!/bin/bash # Check usage of get_llm method rg -nP '\bget_llm\s*\(' --type=py -g '!holmes/core/llm.py'
🧹 Nitpick comments (1)
holmes/core/llm.py (1)
45-46: TODO: Consider removing redundant name field.The TODO comment and a past review comment both note that the
namefield appears redundant and unused except as a pass-through parameter. Consider removing it in a follow-up to simplify theModelEntrystructure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/llm.py(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
holmes/core/llm.py
🧬 Code graph analysis (1)
holmes/core/llm.py (3)
holmes/clients/robusta_client.py (2)
RobustaModelsResponse(17-23)fetch_robusta_models(27-42)holmes/utils/file_utils.py (1)
load_yaml_file(19-56)holmes/utils/env.py (1)
replace_env_vars_values(36-60)
⏰ 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). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (3)
holmes/core/llm.py (3)
60-62: LGTM: @classmethod decorator correctly added.The previous review concern about the missing
@classmethoddecorator has been properly addressed.
434-441: LGTM: api_key and api_version properly forwarded from config.The changes correctly restore
api_keysupport from the configuration file and addapi_versionfor Azure OpenAI Service, directly addressing the regression mentioned in the PR objectives.
528-528: [shell scripts to locate ModelEntry definition]#!/bin/bash # Locate the ModelEntry class definition to verify it's a Pydantic BaseModel subclass rg -n 'class ModelEntry' -C3 --type=py
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holmes/core/llm.py (2)
520-546: Use .model_copy() instead of .copy() for Pydantic v2 models.Pydantic v2 models use
.model_copy()instead of.copy(). The calls at lines 528, 538, and 546 will raiseAttributeErrorat runtime sinceModelEntryis a Pydantic v2 model.Apply this diff to fix all three occurrences:
if model_params is not None: logging.info(f"Using selected model: {model_key}") - return model_params.copy() + return model_params.model_copy() logging.error(f"Couldn't find model: {model_key} in model list") if self._default_robusta_model: model_params = self._llms.get(self._default_robusta_model) if model_params is not None: logging.info( f"Using default Robusta AI model: {self._default_robusta_model}" ) - return model_params.copy() + return model_params.model_copy() logging.error( f"Couldn't find default Robusta AI model: {self._default_robusta_model} in model list" ) model_key, first_model_params = next(iter(self._llms.items())) logging.debug(f"Using first available model: {model_key}") - return first_model_params.copy() + return first_model_params.model_copy()
548-549: Fix get_llm to return an LLM instance, not ModelEntry.The method signature indicates it returns
LLM, but it actually returnsModelEntry(fromself._llms[name]). This type mismatch is suppressed with# type: ignorebut will cause runtime type errors for callers expecting anLLMinstance.The method should likely construct and return a
DefaultLLMinstance from theModelEntry, similar to how it's done elsewhere in the codebase.Consider implementing the method like this:
def get_llm(self, name: str) -> LLM: # TODO: fix logic - return self._llms[name] # type: ignore + model_entry = self._llms[name] + return DefaultLLM( + model=model_entry.model, + api_key=model_entry.api_key.get_secret_value() if model_entry.api_key else None, + api_base=model_entry.api_base or model_entry.base_url, + api_version=model_entry.api_version, + args=model_entry.custom_args or {}, + name=model_entry.name, + )
🧹 Nitpick comments (1)
holmes/core/llm.py (1)
48-54: Consider clarifying the distinction between base_url and api_base.
ModelEntryhas bothbase_urlandapi_basefields, which could be confusing. According to the inline comment,api_baseis for Azure OpenAI Service, but this relationship isn't immediately clear from the field names alone.Consider either:
- Renaming
api_basetoazure_api_baseto make the purpose explicit- Adding a comment on the
base_urlfield explaining how it differs fromapi_base- Consolidating these fields if they serve the same purpose in different contexts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/llm.py(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
holmes/core/llm.py
🧬 Code graph analysis (1)
holmes/core/llm.py (3)
holmes/clients/robusta_client.py (2)
RobustaModelsResponse(17-23)fetch_robusta_models(27-42)holmes/utils/file_utils.py (1)
load_yaml_file(19-56)holmes/utils/env.py (1)
replace_env_vars_values(36-60)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (5)
holmes/core/llm.py (5)
493-498: LGTM!The refactor to create
ModelEntrydirectly instead of a dict is clean and type-safe.
555-564: LGTM!The refactor correctly uses
ModelEntry.model_validate()(Pydantic v2 API) to parse model parameters from the YAML file, and the type annotations are consistent.
566-597: LGTM!Both
_create_robusta_model_entryand_create_model_entryare correctly refactored to returnModelEntryinstances. The addition ofapi_key,api_base, andapi_versionparameters aligns with the PR objective to restore api_key support.
45-46: Verify instantiations and references ofnamebefore removal.Run:
rg -nP 'ModelEntry\([^)]*name=' --type=py rg -nP '\b(entry|model_entry)\.name\b' --type=py rg -nP '\{.*name.*\}' --type=py
434-441: Ignore SecretStr unwrapping concern in DefaultLLM instantiation.The config code unwraps
api_keywith.get_secret_value()before callingDefaultLLM.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
holmes/core/llm.py (1)
41-63: Well-structured model with proper security practices.The
ModelEntryclass provides a clean, type-safe representation of LLM configurations. UsingSecretStrforapi_keyis appropriate for security, andConfigDict(extra="allow")provides forward compatibility.Note: The TODO comment about the redundant
namefield (line 45) has already been raised in previous review comments.
🧹 Nitpick comments (1)
holmes/core/llm.py (1)
531-557: Consider usingmodel_copy()instead of deprecatedcopy().Pydantic v2 deprecated
copy()in favor ofmodel_copy(). Whilecopy()still works for backward compatibility, usingmodel_copy()aligns with current best practices and avoids potential deprecation warnings.Apply this pattern at lines 539, 549, and 557:
-return model_params.copy() +return model_params.model_copy()Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/llm.py(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
holmes/core/llm.py
🧬 Code graph analysis (1)
holmes/core/llm.py (3)
holmes/clients/robusta_client.py (2)
RobustaModelsResponse(17-23)fetch_robusta_models(27-42)holmes/utils/file_utils.py (1)
load_yaml_file(19-56)holmes/utils/env.py (1)
replace_env_vars_values(36-60)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (8)
holmes/core/llm.py (8)
3-3: LGTM!The added imports properly support the new
ModelEntryclass and enhanced type safety. All imports are correctly placed at the top of the file.Also applies to: 6-6, 8-8, 11-13, 17-17
428-428: LGTM!The type annotation change from
dict[str, dict[str, Any]]todict[str, ModelEntry]improves type safety and consistency with the newModelEntrymodel.
445-452: LGTM!The addition of
api_keyandapi_versionparameters correctly restores support for user-configured API keys, addressing the regression from PR #954.
504-509: LGTM!The direct instantiation of
ModelEntryfor the default Robusta configuration is correct and consistent with the refactor.
563-564: LGTM!The return type update is consistent with the
ModelEntryrefactor.
566-575: LGTM!The refactor correctly uses
ModelEntry.model_validate()to create type-safe model entries from YAML data. The environment variable replacement happens before validation, which is the correct order.
577-588: LGTM!The method correctly returns a
ModelEntryand properly handles thecustom_argsfield.
590-608: LGTM!The method signature and implementation correctly support the new
api_key,api_base, andapi_versionparameters. The use ofSecretStrforapi_keymaintains security, and all parameters properly align with theModelEntryfield definitions.
2 participants
We introduced LLM Model Registry in #954 to better manage the LLM model, and removed api_key from Config class, which leads to user configured api_key not working anymore.
This change brings the api_key back to config file, and introduces ModelEntry class to clarify the format of the LLM parameters.