Load default robusta model from API - #946
moshemorad merged 6 commits into
Conversation
WalkthroughAdds typed Robusta model response handling, centralizes model parameter resolution and default Robusta selection in Config, passes model-name into DefaultLLM, extends DefaultLLM to store a name, adds Supabase test mocks and Robusta HTTP/test scaffolding, updates tests, and adds pytest-responses dev dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test
participant Config as Config
participant RobustaClient as Robusta Client
participant ModelList as _model_list
participant LLM as DefaultLLM
rect rgb(250,250,255)
note over Test,RobustaClient: Config load (Robusta AI enabled)
Test->>RobustaClient: fetch_robusta_models(account_id, token)
RobustaClient-->>Test: RobustaModelsResponse(models[], default_model?)
Test->>Config: load_from_env()
Config->>ModelList: configure_robusta_ai_model(robusta_models.models, robusta_models.default_model)
ModelList-->>Config: entries with name, base_url, is_robusta_model, model
Config->>Config: set _default_robusta_model (if provided)
end
rect rgb(245,255,245)
note over Test,Config: Acquire LLM instance
Test->>Config: _get_llm(model_key?)
Config->>Config: _get_model_params(model_key) -> params
alt params.is_robusta_model and Config has global robusta key
Config->>LLM: DefaultLLM(model=params.model, api_key=<robusta_key>, api_base=params.base_url, api_version=params.api_version, args=params.args, tracer, name=params.name)
else
Config->>LLM: DefaultLLM(model=params.model, api_key=params.api_key, api_base=params.api_base, api_version=params.api_version, args=params.args, tracer, name=params.name)
end
LLM-->>Test: DefaultLLM instance (with .name)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the ✨ Finishing Touches
🧪 Generate unit tests
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 |
06a1038 to
103320e
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/config.py (1)
68-75: Environment variable substitution isn’t applied; also guard None.You rebind the loop variable instead of updating the dict. If load_yaml_file returns None, .items() will fail.
Apply:
def parse_models_file(path: str): - models = load_yaml_file(path, raise_error=False, warn_not_found=False) - - for _, params in models.items(): - params = replace_env_vars_values(params) - - return models + models = load_yaml_file(path, raise_error=False, warn_not_found=False) or {} + for k, params in list(models.items()): + models[k] = replace_env_vars_values(params) + return models
🧹 Nitpick comments (16)
holmes/core/llm.py (1)
75-77: New tracer/name args: LGTM; add attribute type hints to satisfy mypy.mypy may complain about dynamically added attributes. Declare class attributes for tracer/name.
Apply this diff near the existing class attributes:
class DefaultLLM(LLM): model: str api_key: Optional[str] api_base: Optional[str] api_version: Optional[str] args: Dict + tracer: Optional[Any] + name: Optional[str]Also applies to: 84-84
holmes/clients/robusta_client.py (1)
9-10: Unify timeouts via a constant.POST uses a literal 10s while GET uses TIMEOUT=0.5s. Prefer explicit POST timeout constant.
Apply:
-HOLMES_GET_INFO_URL = f"{ROBUSTA_API_ENDPOINT}/api/holmes/get_info" -TIMEOUT = 0.5 +HOLMES_GET_INFO_URL = f"{ROBUSTA_API_ENDPOINT}/api/holmes/get_info" +TIMEOUT = 0.5 +POST_TIMEOUT = 10.0 @@ - resp = requests.post( + resp = requests.post( f"{ROBUSTA_API_ENDPOINT}/api/llm/models", json=session_request, - timeout=10, + timeout=POST_TIMEOUT, )Also applies to: 27-31
tests/plugins/toolsets/test_internet.py (1)
115-120: Mock response: add content-type header for fidelity.Set Content-Type to mimic real HTML.
Apply:
- responses.get( + responses.get( TEST_URL, status=200, - body=EXPECTED_TEST_RESULT, + body=EXPECTED_TEST_RESULT, + headers={"Content-Type": "text/html; charset=utf-8"}, )tests/config_class/test_config_load_robusta_ai.py (1)
10-14: Remove unused helper.
fake_load_robusta_api_keyis defined but never used; delete to avoid confusion.-def fake_load_robusta_api_key(config, _): - config.account_id = "mock-account" - config.session_token = SecretStr("mock-token") - config.api_key = SecretStr("mock-token") -tests/config_class/test_config_get_llm.py (2)
14-23: Typo in test name.Rename
test_confgi_get_llm_with_model_key_returns_model_from_config.-def test_confgi_get_llm_with_model_key_returns_model_from_config( +def test_config_get_llm_with_model_key_returns_model_from_config(
7-12: Avoid hardcoding Robusta API base URL.Use ROBUSTA_API_ENDPOINT for stability across environments.
-from tests.conftest import DEFAULT_ROBUSTA_MODEL +from tests.conftest import DEFAULT_ROBUSTA_MODEL +from holmes.common.env_vars import ROBUSTA_API_ENDPOINT @@ - assert llm.api_base == f"https://api.robusta.dev/llm/{DEFAULT_ROBUSTA_MODEL}" + assert llm.api_base == f"{ROBUSTA_API_ENDPOINT}/llm/{DEFAULT_ROBUSTA_MODEL}" @@ - assert llm.api_base == f"https://api.robusta.dev/llm/{DEFAULT_ROBUSTA_MODEL}" + assert llm.api_base == f"{ROBUSTA_API_ENDPOINT}/llm/{DEFAULT_ROBUSTA_MODEL}"Also applies to: 25-32
tests/config_class/test_config_api_base_version.py (2)
51-56: Add assertion for model_name (7th arg) to prevent regressions.You’re already verifying args 0–5. Also assert the name forwarded to DefaultLLM.
Apply:
- # Check that DefaultLLM was called with the right positional arguments + # Check that DefaultLLM was called with the right positional arguments call_args = mock_default_llm.call_args[0] assert call_args[0] == "test-model" assert call_args[1] is None assert call_args[2] == "https://test.api.base" assert call_args[3] == "2023-12-01" assert call_args[4] == {} assert call_args[5] is None # tracer + assert call_args[6] == "test-model"
200-208: Consider stabilizing the “name” semantics.Here the 7th arg becomes the model id ("gpt-4") rather than the model-list key (“first-model”), unlike other tests. If you want consistent labels for tracing/metrics, set a “name” in the model-list entry or change _get_llm to prefer the key when no explicit name exists.
Example change to the test data:
- "first-model": { + "first-model": { + "name": "first-model", "model": "gpt-4",Or adjust _get_llm to compute name as: model_key or name or model.
tests/conftest.py (3)
15-24: Don’t swallow all exceptions when clearing caches.Silent failures can mask setup issues. Log at debug at least, or narrow to AttributeError/ImportError.
Apply:
- except Exception: - pass + except (AttributeError, ImportError) as e: + import logging + logging.debug("Cache clear skipped: %s", e)
27-35: Mocked Robusta models may never be used in current fixture flow.With a user-provided model list and default LOAD_ALL_ROBUSTA_MODELS=False, configure_robusta_ai_model won’t call fetch_robusta_models, so this responses stub is unused. Either enable loading or drop the stub to speed tests.
Option A (enable):
monkeypatch.setattr("holmes.config.ROBUSTA_AI", True) + monkeypatch.setattr("holmes.config.LOAD_ALL_ROBUSTA_MODELS", True)Option B: remove the responses.post in this fixture and keep it in tests that actually rely on it.
44-49: Fixture intent clarification.Setting ROBUSTA_AI=True while also providing a local model list disables Robusta auto-loading (_should_load_robusta_ai returns False). If that’s intended, add a brief comment for future readers.
holmes/config.py (5)
151-154: Ensure _model_list is always a dict.Be defensive in case parse_models_file ever returns None.
Apply:
- self._model_list = parse_models_file(MODEL_LIST_FILE_LOCATION) + self._model_list = parse_models_file(MODEL_LIST_FILE_LOCATION) or {}
180-188: Hardcoding "model": "gpt-4o" for all Robusta entries.If server returns a canonical underlying model, prefer that; otherwise fine as a default.
200-208: Default Robusta base_url shape inconsistent with per-model entries.Per-model uses f"{ROBUSTA_API_ENDPOINT}/llm/{model}", default uses ROBUSTA_API_ENDPOINT only. If the endpoint requires /llm routing, default may differ at runtime.
Option:
- self._model_list[ROBUSTA_AI_MODEL_NAME] = { + self._model_list[ROBUSTA_AI_MODEL_NAME] = { "name": ROBUSTA_AI_MODEL_NAME, - "base_url": ROBUSTA_API_ENDPOINT, + "base_url": f"{ROBUSTA_API_ENDPOINT}/llm/{self.model or 'gpt-4o'}", "is_robusta_model": True, }
571-577: Robusta API key handling: fail-fast if missing.If a Robusta entry is chosen but api_key isn’t present on self (and none in model_params), we’ll pass None to DefaultLLM. Prefer an explicit error with guidance.
Apply:
- if is_robusta_model and self.api_key: + if is_robusta_model and self.api_key: # we set here the api_key since it is being refresh when exprided and not as part of the model loading. api_key = self.api_key.get_secret_value() # type: ignore else: api_key = model_params.pop("api_key", None) + if is_robusta_model and not api_key: + raise ValueError("Robusta model selected but no API key available")
585-589: Stabilize name computation (telemetry consistency).Prefer model_key over “name” to keep labels tied to the model-list key; fall back to name/model.
Apply:
- model_name = model_params.pop("name", None) or model_key or model + model_name = model_key or model_params.pop("name", None) or model
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ec4307a and 78bcc1621aad107472d5af9f33be4760a638f6a8.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
conftest.py(2 hunks)holmes/clients/robusta_client.py(2 hunks)holmes/config.py(4 hunks)holmes/core/llm.py(1 hunks)pyproject.toml(1 hunks)tests/config_class/test_config_api_base_version.py(9 hunks)tests/config_class/test_config_get_llm.py(1 hunks)tests/config_class/test_config_load_model_list.py(2 hunks)tests/config_class/test_config_load_robusta_ai.py(5 hunks)tests/conftest.py(1 hunks)tests/plugins/toolsets/test_internet.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
tests/config_class/test_config_load_model_list.pytests/plugins/toolsets/test_internet.pyholmes/clients/robusta_client.pytests/config_class/test_config_get_llm.pytests/conftest.pyconftest.pyholmes/core/llm.pytests/config_class/test_config_load_robusta_ai.pytests/config_class/test_config_api_base_version.pyholmes/config.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use invalid pytest markers; only use markers/tags declared in pyproject.toml
Files:
tests/config_class/test_config_load_model_list.pytests/plugins/toolsets/test_internet.pytests/config_class/test_config_get_llm.pytests/conftest.pytests/config_class/test_config_load_robusta_ai.pytests/config_class/test_config_api_base_version.py
🧠 Learnings (1)
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#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/config_class/test_config_load_model_list.py
🔇 Additional comments (13)
pyproject.toml (1)
84-84: Add pytest-responses: looks good; verify compatibility matrix.Plugin version ^0.5.1 generally pairs with responses ≥0.23 and pytest ≥7. Please double-check in CI that pytest (8.3.3) + responses (0.23.1) + pytest-responses (0.5.1) work together across all runners. If any flake appears, consider bumping responses to the latest 0.24.x in lockstep.
holmes/clients/robusta_client.py (1)
17-21: Typed response model: good change.Using a Pydantic response object makes downstream usage safer and clearer.
conftest.py (1)
133-147: Autouse service patches: LGTM.Good isolation of external deps (Supabase + credentials) without leaking real values.
Also applies to: 149-159
tests/config_class/test_config_load_robusta_ai.py (1)
4-8: Adapting to RobustaModelsResponse: LGTM.Switching patches to return the typed response keeps tests aligned with client changes.
tests/config_class/test_config_api_base_version.py (7)
81-89: LLM call expectations look correct.The model-list key is forwarded as the 7th arg and tracer is None.
118-126: Model-list overrides over config are asserted correctly.Good precedence checks and name forwarding.
153-161: Fallback to config values is validated well.Covers missing api_base/api_version in model list.
255-263: Specific model selection from model list is correctly asserted.Arguments and name forwarding (“openai-gpt35”) look good.
289-297: base_url → api_base mapping verified.Covers legacy field support without ambiguity.
324-332: api_base precedence over base_url is asserted correctly.
360-368: Config api_base fallback is asserted correctly.holmes/config.py (2)
173-179: Network fetch path: OK with broad try/except; consider timeout/backoff at source.Assuming fetch_robusta_models handles timeouts/retries. If not, add them there.
537-565: Model resolution flow: solid.Good handling of explicit key, default Robusta model, then first model fallback with copies.
Avi-Robusta
left a comment
Avi-Robusta
left a comment
There was a problem hiding this comment.
Looks good, two small changes
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
holmes/config.py (3)
173-194: Robusta models: typed response handling and default selection look solid.
- Using the structured response and guarding with
if not robusta_models or not robusta_models.models:matches the prior suggestion and prevents crashes.- Persisting
default_modelinto_default_robusta_modelhelps centralize selection.Optional: model names contain spaces (e.g., “sonnet-4 preview”). If these are ever used as URL path segments beyond
base_url, ensure the HTTP client safely encodes them. No change requested here since tests assert the current form.
180-187: Include a concrete “model” for Robusta entries.Setting
"model": "gpt-4o"for Robusta entries and adding"name"are both correct and align with the new selection flow.
200-209: Set the default Robusta model when only the single default is loaded.This keeps
_get_model_paramsdeterministic (it won’t fall back to “first model” ordering) and addresses the earlier ask to persist the default model key.def _load_default_robusta_config(self): if self._should_load_robusta_ai() and self.api_key: logging.info("Loading default Robusta AI model") self._model_list[ROBUSTA_AI_MODEL_NAME] = { "name": ROBUSTA_AI_MODEL_NAME, "base_url": ROBUSTA_API_ENDPOINT, "is_robusta_model": True, "model": "gpt-4o", } + # Keep selection logic consistent with multi-model flows + self._default_robusta_model = ROBUSTA_AI_MODEL_NAME
🧹 Nitpick comments (3)
holmes/config.py (3)
137-137: Track and actually use the default Robusta model key.Good adding
_default_robusta_model. To fully leverage it, also set it when loading the single default Robusta config so_get_model_paramsprefers it.See diff in the
_load_default_robusta_configcomment below.
538-566: Tighten return typing for_get_model_params.Return type is too generic for mypy config; prefer
dict[str, Any]. You already importAny.-def _get_model_params(self, model_key: Optional[str] = None) -> dict: +def _get_model_params(self, model_key: Optional[str] = None) -> dict[str, Any]:
567-591: Add type for tracer and fix a minor comment typo.
- Annotate
tracerto satisfy the “type hints required” rule.- Typo: “exprided” → “expired”.
-def _get_llm(self, model_key: Optional[str] = None, tracer=None) -> "DefaultLLM": +def _get_llm(self, model_key: Optional[str] = None, tracer: Optional[Any] = None) -> "DefaultLLM": @@ - if is_robusta_model and self.api_key: - # we set here the api_key since it is being refresh when exprided and not as part of the model loading. + if is_robusta_model and self.api_key: + # we set the api_key here since it is refreshed when expired, not at model loading time. api_key = self.api_key.get_secret_value() # type: ignoreHeads-up: for non-Robusta models without a model-list entry,
api_keyis nowNone(by design per tests). Please ensure downstreamDefaultLLMcontinues to pick credentials from env as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 78bcc1621aad107472d5af9f33be4760a638f6a8 and ff07c37e8ab63aa6024656ef6d0283e32b197e95.
📒 Files selected for processing (2)
holmes/clients/robusta_client.py(2 hunks)holmes/config.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/clients/robusta_client.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
holmes/config.py
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
holmes/config.py (3)
173-179: Defensive fallback looks good.
fetch_robusta_models(...)guarded withif not robusta_models or not robusta_models.models:addresses the empty/None response path.
180-187: Robusta models ingestion: shape looks correct.Including
"name","is_robusta_model", and"model": "gpt-4o"per prior feedback is solid.
200-210: Default Robusta model entry is well-formed.Adding
"name"and"model": "gpt-4o"and setting_default_robusta_model = ROBUSTA_AI_MODEL_NAMEresolves prior gaps.
🧹 Nitpick comments (3)
holmes/config.py (3)
137-137: Make this a private attribute (Pydantic v2).Declare
_default_robusta_modelas aPrivateAttrso it won’t appear in.model_dump()or validation.Apply within this range:
- _default_robusta_model: Optional[str] = None + _default_robusta_model: Optional[str] = PrivateAttr(default=None)Also add the import at the top:
from pydantic import BaseModel, ConfigDict, FilePath, SecretStr, PrivateAttr
539-567: Centralized model resolution: add precise return type and improve fallback logging.
- Add
dict[str, Any]return type for mypy.- Log the actual first model key to aid debugging.
- def _get_model_params(self, model_key: Optional[str] = None) -> dict: + def _get_model_params(self, model_key: Optional[str] = None) -> dict[str, Any]: @@ - first_model_params = next(iter(self._model_list.values())).copy() - logging.info("Using first model") - return first_model_params + first_key, first_model_params = next(iter(self._model_list.items())) + logging.info(f"Using first model: {first_key}") + return first_model_params.copy()
575-577: Typo in comment.“exprided” → “expired”; tighten the sentence.
- # we set here the api_key since it is being refresh when exprided and not as part of the model loading. + # set api_key here because it is refreshed when expired, not as part of model loading.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ff07c37e8ab63aa6024656ef6d0283e32b197e95 and 6f5321b140491c53d82af932cb64017f456e0cb5.
📒 Files selected for processing (1)
holmes/config.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
holmes/config.py
⏰ 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.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: llm_evals
🔇 Additional comments (3)
holmes/config.py (3)
180-187: Verify base_url path consistency.Here you use
base_url=f"{ROBUSTA_API_ENDPOINT}/llm/{model}", while the default config below usesbase_url=ROBUSTA_API_ENDPOINT. Confirm the server expects per-model routing on/llm/{model}and consider aligning the default path for consistency.Would you like a patch that standardizes both paths?
189-194: Persisting the default Robusta model is correct.Storing
robusta_models.default_modelinto_default_robusta_modelenables sane resolution in_get_model_params.
200-210: Double-check default base_url vs. multi-model base_url.If per-model endpoints are required, consider:
- "base_url": ROBUSTA_API_ENDPOINT, + "base_url": f"{ROBUSTA_API_ENDPOINT}/llm/{ROBUSTA_AI_MODEL_NAME}",Otherwise, document why the default uses the root while enumerated models use
/llm/{model}.
6f5321b to
eb7a4f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
holmes/config.py (1)
568-592: Non-Robusta api_key fallback likely lost — restore global/env key.When not a Robusta model, api_key now depends solely on model_params and may drop env/global Config.api_key. This can break non-Robusta usage. Also fix a small comment typo and annotate tracer.
Apply:
- def _get_llm(self, model_key: Optional[str] = None, tracer=None) -> "DefaultLLM": + def _get_llm(self, model_key: Optional[str] = None, tracer: Optional[Any] = None) -> "DefaultLLM": @@ - if is_robusta_model and self.api_key: - # we set here the api_key since it is being refresh when exprided and not as part of the model loading. + if is_robusta_model and self.api_key: + # set api_key here since it is refreshed when expired and not as part of model loading. api_key = self.api_key.get_secret_value() # type: ignore else: - api_key = model_params.pop("api_key", None) + # Prefer explicit model-level key; fall back to global Config.api_key (may come from env) + api_key = model_params.pop("api_key", None) or ( + self.api_key.get_secret_value() if self.api_key else None + )#!/bin/bash # Sanity: confirm DefaultLLM accepts the added model_name arg and no internal api_key override exists. rg -n -C3 $'class\\s+DefaultLLM\\b|def\\s+__init__\\s*\\(' holmes/core/llm.py
🧹 Nitpick comments (2)
holmes/config.py (2)
180-187: Avoid double slashes in constructed base_url.If ROBUSTA_API_ENDPOINT ends with '/', this will yield '//llm/...'. Minor but easy to harden.
Apply:
- "base_url": f"{ROBUSTA_API_ENDPOINT}/llm/{model}", + "base_url": f"{ROBUSTA_API_ENDPOINT.rstrip('/')}/llm/{model}",
539-567: Tighten typing and improve “first model” log.Return type should be dict[str, Any], and logging the chosen first model aids debugging.
Apply:
- def _get_model_params(self, model_key: Optional[str] = None) -> dict: + def _get_model_params(self, model_key: Optional[str] = None) -> dict[str, Any]: @@ - first_model_params = next(iter(self._model_list.values())).copy() - logging.info("Using first model") - return first_model_params + first_key = next(iter(self._model_list.keys())) + first_model_params = self._model_list[first_key].copy() + logging.info(f"Using first model: {first_key}") + return first_model_params
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6f5321b140491c53d82af932cb64017f456e0cb5 and eb7a4f7.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
conftest.py(2 hunks)holmes/clients/robusta_client.py(2 hunks)holmes/config.py(4 hunks)holmes/core/llm.py(1 hunks)pyproject.toml(1 hunks)tests/config_class/test_config_api_base_version.py(9 hunks)tests/config_class/test_config_get_llm.py(1 hunks)tests/config_class/test_config_load_model_list.py(2 hunks)tests/config_class/test_config_load_robusta_ai.py(5 hunks)tests/conftest.py(1 hunks)tests/plugins/toolsets/test_internet.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- holmes/core/llm.py
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/plugins/toolsets/test_internet.py
- holmes/clients/robusta_client.py
- pyproject.toml
- tests/config_class/test_config_get_llm.py
- conftest.py
- tests/config_class/test_config_load_model_list.py
- tests/config_class/test_config_load_robusta_ai.py
- tests/conftest.py
- tests/config_class/test_config_api_base_version.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
holmes/config.py
🧠 Learnings (1)
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#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:
holmes/config.py
⏰ 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.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: llm_evals
🔇 Additional comments (4)
holmes/config.py (4)
137-137: Default Robusta model slot added — LGTM.This enables consistent fallback/selection across the new resolution path.
173-179: Typed fetch + empty-list fallback — LGTM.Good use of the typed response and graceful fallback to default config when models are missing.
189-194: Persisting server-provided default — LGTM.Storing and later honoring default_model is the right call.
204-210: Confirm default Robusta base_url path consistency.Multi-model entries use “…/llm/{model}” but the single default uses the endpoint root. Verify the server expects root here; if not, switch to the llm path.
If needed:
- "base_url": ROBUSTA_API_ENDPOINT, + "base_url": f"{ROBUSTA_API_ENDPOINT.rstrip('/')}/llm/{ROBUSTA_AI_MODEL_NAME}",
…lected-default-model-from-setting
2 participants
No description provided.