[LEADS-415] Responses streaming support#255
Conversation
|
Warning Review limit reached
More reviews will be available in 54 minutes and 27 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe PR refactors the streaming parser from Pydantic models to dataclasses and replaces the monolithic event switch with handler-dispatch tables. It adds a new ChangesResponses Endpoint Streaming Support
Example README Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
xmican10
force-pushed
the
LEADS-415-responses-streaming-support
branch
3 times, most recently
from
e8a6dd9 to
5bb24c3
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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
🧹 Nitpick comments (1)
src/lightspeed_evaluation/core/api/streaming_parser.py (1)
356-358: 💤 Low valueUnreachable code:
argumentscan never beNoneafter line 350.Line 350 uses
or {}as a fallback, soargumentswill always be at least an empty dict. The conditionif arguments is Noneon line 356 will never be true.Consider removing this unreachable block:
Proposed fix
if not tool_name: logger.debug("Tool call missing name/tool_name field") return None - if arguments is None: - logger.debug("Tool call missing args/arguments field for %s", tool_name) - return None - tool_call: dict[str, Any] = {"tool_name": tool_name, "arguments": arguments}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lightspeed_evaluation/core/api/streaming_parser.py` around lines 356 - 358, Remove the unreachable `if arguments is None:` condition and its associated debug logging block. Since the arguments variable is assigned with an `or {}` fallback pattern earlier in the code, it will always contain at least an empty dictionary and can never be None, making this entire conditional block dead code that should be deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/core/api/test_client_responses.py`:
- Line 1: Remove the module-level pylint disable comment at the top of
test_client_responses.py that suppresses protected-access and duplicate-code
warnings. Instead of disabling these checks globally, identify the specific code
locations that trigger these warnings (likely places where protected members are
accessed with underscore prefixes or where code is duplicated) and either
refactor the code to avoid the issue, or if necessary, apply targeted localized
lint suppressions directly to those specific lines. This ensures code quality
standards are maintained and warnings are addressed rather than hidden.
---
Nitpick comments:
In `@src/lightspeed_evaluation/core/api/streaming_parser.py`:
- Around line 356-358: Remove the unreachable `if arguments is None:` condition
and its associated debug logging block. Since the arguments variable is assigned
with an `or {}` fallback pattern earlier in the code, it will always contain at
least an empty dictionary and can never be None, making this entire conditional
block dead code that should be deleted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00d2cc99-273a-4e6c-8365-056cd39844d9
📒 Files selected for processing (19)
config/system.yamlexamples/01_getting_started/basic_setup/README.mdexamples/02_metrics/context_quality/README.mdexamples/02_metrics/conversation_quality/README.mdexamples/02_metrics/keywords_evaluation/README.mdexamples/02_metrics/nlp_metrics/README.mdexamples/02_metrics/response_quality/README.mdexamples/02_metrics/tool_evaluation/README.mdexamples/03_endpoints/responses/README.mdexamples/03_endpoints/responses/eval_data.yamlexamples/03_endpoints/responses/system.yamlsrc/lightspeed_evaluation/core/api/client.pysrc/lightspeed_evaluation/core/api/streaming_parser.pysrc/lightspeed_evaluation/core/constants.pysrc/lightspeed_evaluation/core/metrics/custom/tool_eval.pytests/unit/core/api/conftest.pytests/unit/core/api/test_client_responses.pytests/unit/core/api/test_streaming_parser.pytests/unit/core/metrics/custom/test_tool_eval.py
| @@ -0,0 +1,386 @@ | |||
| # pylint: disable=protected-access,duplicate-code | |||
There was a problem hiding this comment.
Remove module-level lint suppression and fix the underlying warnings.
Line 1 disables pylint checks for the whole module. Please remove this directive and resolve the concrete warnings directly (or localize/refactor the triggering code) instead of suppressing them globally.
As per coding guidelines, "**/*.py: Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/core/api/test_client_responses.py` at line 1, Remove the
module-level pylint disable comment at the top of test_client_responses.py that
suppresses protected-access and duplicate-code warnings. Instead of disabling
these checks globally, identify the specific code locations that trigger these
warnings (likely places where protected members are accessed with underscore
prefixes or where code is duplicated) and either refactor the code to avoid the
issue, or if necessary, apply targeted localized lint suppressions directly to
those specific lines. This ensures code quality standards are maintained and
warnings are addressed rather than hidden.
Source: Coding guidelines
|
I need to rebase.. |
xmican10
force-pushed
the
LEADS-415-responses-streaming-support
branch
from
5bb24c3 to
8c03464
Compare
xmican10
force-pushed
the
LEADS-415-responses-streaming-support
branch
from
8c03464 to
54e60be
Compare
|
@coderabbitai full review |
asamal4
left a comment
asamal4
left a comment
There was a problem hiding this comment.
Thanks !! Some minor comments - applicable in multiples places
| handler(ctx, item) | ||
|
|
||
|
|
||
| def _responses_handle_mcp_call_item( |
There was a problem hiding this comment.
didn't we add similar logic in _parse_mcp_call_item
| Dictionary with tool_name, arguments, and optional result, or None if invalid. | ||
| """ | ||
| try: | ||
| # Support both "name"/"args" (new format) and "tool_name"/"arguments" (legacy) |
There was a problem hiding this comment.
Why are we removing these comments ?
| 'data: {"event": "start", "data": {"conversation_id": "conv_new"}}', | ||
| 'data: {"event": "tool_call", "data": ' | ||
| '{"id": "tc_1", "name": "pods_list", "args": {"namespace": "default"}}}', | ||
| '{"id": "tc_1", "name": "get_weather", "args": {"city": "London"}}}', |
There was a problem hiding this comment.
Why are we changing this ? This is just test data - but still I would prefer to keep it more realistic + there is no technical reason for chaning this.
| '{"id": "tc_1", "name": "get_weather", "args": {"city": "London"}}}', | |
| '{"id": "tc_1", "name": "pods_list", "args": {"namespace": "default"}}}', |
2 participants
Description
Followup PR with supporting responses
stream=TrueparameterType of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
/responsesendpoint, enabling real-time response streaming for improved latency.Documentation
--output-dirflag for evaluation results.