ROB-1689 call_stream support for api chat - #759
Conversation
WalkthroughA new boolean Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
holmes/core/models.py(2 hunks)holmes/core/tool_calling_llm.py(5 hunks)holmes/utils/stream.py(1 hunks)server.py(3 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)
Type hints required (mypy configuration in pyproject.toml)
Files:
server.pyholmes/core/models.pyholmes/utils/stream.pyholmes/core/tool_calling_llm.py
🧠 Learnings (2)
server.py (1)
Learnt from: nherment
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.
holmes/core/models.py (1)
Learnt from: nherment
PR: #436
File: tests/llm/utils/mock_utils.py:240-249
Timestamp: 2025-06-05T12:23:27.634Z
Learning: The holmesgpt project uses Python >= 3.10 and prefers modern type hint syntax like list[str], dict[str, int] over importing equivalent types from the typing module like List[str], Dict[str, int].
🧬 Code Graph Analysis (1)
holmes/utils/stream.py (1)
holmes/core/tool_calling_llm.py (1)
call_stream(544-676)
🪛 Ruff (0.12.2)
holmes/utils/stream.py
19-19: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
holmes/core/tool_calling_llm.py
606-608: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ 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: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
🔇 Additional comments (10)
holmes/core/models.py (1)
92-92: LGTM! Clean addition of streaming support.The
streamfield addition toChatRequestBaseModelis well-implemented with appropriate typing and default value. This enables clients to opt into streaming responses, which aligns perfectly with the PR objectives.server.py (2)
26-26: LGTM! Import of streaming utilities.The import of streaming formatters enables the server endpoints to use the new structured streaming approach.
162-174: LGTM! Clean refactoring to use structured streaming.The streaming investigation endpoint now uses the new
stream_investigate_formatterwhich properly handles the structuredStreamMessageobjects fromai.call_stream. The runbooks parameter is correctly passed for inclusion in the output.holmes/utils/stream.py (3)
8-17: LGTM! Well-designed streaming message structure.The
StreamEventsenum andStreamMessagemodel provide a clean, structured approach to handling streaming events. This separation of concerns improves maintainability compared to the previous direct SSE string generation.
23-42: LGTM! Clean investigation stream formatting.The
stream_investigate_formatterproperly processesANSWER_ENDevents by extracting structured sections and analysis using the existingprocess_response_into_sectionsfunction. The runbooks parameter is correctly included in the output.
44-60: LGTM! Clean chat stream formatting.The
stream_chat_formatterproperly handles chat-specific streaming by including analysis, conversation history, and follow-up actions in the response. The optional followups parameter provides good flexibility.holmes/core/tool_calling_llm.py (4)
38-38: LGTM! Import of streaming utilities.The import enables the method to use the new structured streaming events.
544-563: LGTM! Clean method signature and documentation.The method signature is well-updated to use
msgsparameter instead of the previousrunbooksparameter, and the documentation clearly explains that this doesn't usestream=Truebut streams Holmes iterations instead.
637-642: LGTM! Clean structured event generation.The method now yields structured
StreamMessageobjects withANSWER_ENDevents containing both content and messages. This is much cleaner than the previous SSE string generation approach.
656-672: LGTM! Well-structured tool event generation.The method properly yields
START_TOOLandTOOL_RESULTevents during tool execution, providing good visibility into the AI's progress. The concurrent execution is preserved while adding streaming capabilities.
# Conflicts: # holmes/core/tool_calling_llm.py
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
holmes/core/tool_calling_llm.py (1)
579-579: Tool disabling logic still uses wrong variable.This is the same issue flagged in previous reviews - the condition should use the local
max_stepsvariable instead ofself.max_stepsto respect any adjustments made during retry logic.Apply this fix:
- tools = None if i == max_steps else tools + tools = None if i == max_steps else toolsWait, I notice the code already shows
i == max_stepson line 579, but the past review comment mentionedi == self.max_steps - 1. Let me check if this has actually been fixed.
🧹 Nitpick comments (1)
holmes/core/tool_calling_llm.py (1)
679-679: Consider using local max_steps variable in error message.For consistency and accuracy, consider using the local
max_stepsvariable instead ofself.max_stepsin the error message, especially sincemax_stepscan be incremented during retry logic.- raise Exception( - f"Too many LLM calls - exceeded max_steps: {i}/{self.max_steps}" - ) + raise Exception( + f"Too many LLM calls - exceeded max_steps: {i}/{max_steps}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/tool_calling_llm.py(5 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)
Type hints required (mypy configuration in pyproject.toml)
Files:
holmes/core/tool_calling_llm.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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (8)
holmes/core/tool_calling_llm.py (8)
41-41: LGTM - Import supports new streaming architecture.The import of
StreamEventsandStreamMessagecorrectly supports the refactored streaming implementation that yields structured objects instead of raw SSE strings.
550-554: LGTM - Method signature aligns with new streaming architecture.The signature changes are well-designed:
- Removing
streamparameter since this method always streams structured events- Adding
msgsparameter for flexible message construction- Making
system_promptoptional supports different use cases- Removing
runbooksshifts responsibility appropriately to callers
556-566: LGTM - Flexible message construction.The new message construction logic cleanly handles different input scenarios - system prompt, user prompt, and pre-constructed messages. This provides good flexibility for various use cases.
596-603: LGTM - Simplified LLM completion call.The completion call is now simplified to always use
stream=False, which aligns with the new architecture of streaming at the iteration level rather than at the LLM response level.
612-614: LGTM - Exception handling preserves context.The exception handling has been correctly updated to use
from eto preserve the original exception context, addressing the previous review feedback.
641-645: LGTM - Clean streaming event generation.The method now yields structured
StreamMessageobjects withANSWER_ENDevents containing both content and message history. This is a clean design that separates concerns between event generation and SSE formatting.
660-663: LGTM - Tool start event generation.Yielding
START_TOOLevents when tools are submitted provides good visibility into tool execution progress for streaming consumers.
673-676: LGTM - Tool result event generation.Yielding
TOOL_RESULTevents with structured tool results provides comprehensive information about tool execution outcomes to streaming consumers.
RoiGlinik
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
holmes/core/tool_calling_llm.py (1)
681-683: Verify exception message consistency.The exception message uses both
iandself.max_stepswhich could be confusing. Consider using the localmax_stepsvariable for consistency with the rest of the method.raise Exception( - f"Too many LLM calls - exceeded max_steps: {i}/{self.max_steps}" + f"Too many LLM calls - exceeded max_steps: {i}/{max_steps}" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/core/models.py(2 hunks)holmes/core/tool_calling_llm.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/core/models.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
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
holmes/core/tool_calling_llm.py
🧠 Learnings (1)
📚 Learning: the robusta-dev/holmesgpt codebase has comprehensive existing validation for azure environment varia...
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/core/tool_calling_llm.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). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (10)
holmes/core/tool_calling_llm.py (10)
15-16: LGTM - Import organization follows coding guidelines.The imports are properly placed at the top of the file as required by the coding guidelines.
41-41: LGTM - New streaming utilities import.The import of
StreamEventsandStreamMessagealigns with the refactoring to use structured streaming objects instead of raw SSE strings.
552-556: Method signature change improves flexibility.The removal of the
streamparameter and replacement ofrunbookswithmsgsmakes the method more generic and allows for different message structures to be passed in.
558-568: Clear documentation and improved message construction.The docstring clearly explains that this doesn't use
llm.completion(stream=true)but streams iterations instead. The conditional message construction logic is well-structured and handles different input scenarios properly.
570-575: Variable initialization aligns with refactoring.The use of
tool_calls: list[dict] = []with explicit type annotation andmax_steps = self.max_stepslocal variable assignment is consistent with the streaming refactoring approach.
581-582: Tool disabling logic is correct.The condition
tools = None if i == max_steps else toolscorrectly disables tools on the final iteration, which addresses the previous review concern about max_steps consistency. The logic now matches the non-streaming version.
598-617: Exception handling properly preserves context.The addition of
from ein the exception chain at line 615 correctly addresses the previous review feedback about preserving exception context. The non-streaming LLM completion call is appropriate for this iteration-based streaming approach.
619-648: Structured streaming implementation is well-designed.The refactoring to yield
StreamMessageobjects with appropriate event types (ANSWER_END) instead of raw SSE strings provides better abstraction and flexibility. The early return when no tool calls are present is efficient and correct.
663-666: Tool start event streaming is well-implemented.Yielding
StreamMessagewithSTART_TOOLevent immediately when submitting tool execution provides real-time feedback to the client about tool execution progress.
676-679: Tool result streaming completes the event flow.The
TOOL_RESULTevent withas_streaming_tool_result_response()data provides comprehensive tool execution feedback and maintains consistency with the streaming architecture.
2 participants
improve and simplify stream_call function
make stream message more flexible for other return formats.