fix: disable streaming by default in sync chat methods (fixes #1733)#1734
fix: disable streaming by default in sync chat methods (fixes #1733)#1734praisonai-triage-agent[bot] wants to merge 6 commits into
Conversation
Fixes the 'Streaming is not supported in sync OpenAIAdapter' error when using multi-agent functionality with Deepseek provider. Root cause: - _chat_completion() and _execute_unified_chat_completion() methods in chat_mixin.py defaulted to stream=True - This caused sync OpenAIAdapter to reject requests with streaming enabled - Multi-agent workflows use sync execution path, triggering the error Changes: - Changed stream=True to stream=False as default in both sync methods - Sync adapters now receive non-streaming requests by default - Async methods can still use streaming when explicitly requested - Maintains backward compatibility as streaming was already broken in sync context Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR fixes a multi-agent startup error by changing two ChangesMulti-agent streaming behavior
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR fixes a
Confidence Score: 5/5Safe to merge — the two-line default change closes a clear error path without disrupting any call-site that explicitly passes stream. Every caller that needs streaming already passes No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant C as agent.chat()
participant CC as _chat_completion(stream=False)
participant UC as _execute_unified_chat_completion(stream=False)
participant D as UnifiedLLMDispatcher
participant A as OpenAIAdapter (sync)
C->>CC: "stream=self.stream (explicit)"
CC->>UC: "stream=stream (explicit)"
UC->>D: "chat_completion(stream=False, ...)"
D->>A: "adapter.chat_completion(stream=False, ...)"
Note over A: kwargs.get('stream', False) == False → no ValueError raised
A-->>D: ChatCompletion response
D-->>UC: response
UC-->>CC: response
CC-->>C: response
Reviews (3): Last reviewed commit: "docs: clarify async/sync stream default ..." | Re-trigger Greptile |
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/c4a541cc-a3dc-4637-85d9-53f7090dcb35 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/c4a541cc-a3dc-4637-85d9-53f7090dcb35 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
![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
🤖 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 `@test_multiagent_fix.py`:
- Around line 29-43: The validation script currently prints pass/fail messages
but always exits zero; update the checks for _chat_completion and
ChatMixin._execute_unified_chat_completion so that when either stream default is
not False the script records failure and exits with a non-zero status (e.g.,
call sys.exit(1) or set a failure flag and exit after checks). Locate the
inspection logic around symbols _chat_completion and
ChatMixin._execute_unified_chat_completion in test_multiagent_fix.py (also the
similar check around line 51) and ensure any failure path triggers a non-zero
exit so CI will fail on regressions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffae35be-51c1-4552-8d8a-60ca4fdb0747
📒 Files selected for processing (2)
src/praisonai-agents/praisonaiagents/agent/chat_mixin.pytest_multiagent_fix.py
| if stream_param and stream_param.default == False: | ||
| print("✓ _chat_completion method has stream=False as default") | ||
| else: | ||
| print(f"✗ _chat_completion method stream default is: {stream_param.default}") | ||
|
|
||
| # Check _execute_unified_chat_completion method | ||
| unified_chat_sig = inspect.signature(ChatMixin._execute_unified_chat_completion) | ||
| unified_stream_param = unified_chat_sig.parameters.get('stream') | ||
| if unified_stream_param and unified_stream_param.default == False: | ||
| print("✓ _execute_unified_chat_completion method has stream=False as default") | ||
| else: | ||
| print(f"✗ _execute_unified_chat_completion method stream default is: {unified_stream_param.default}") | ||
|
|
||
| print("✓ Fix validation completed - sync methods now default to stream=False") | ||
| print("✓ This should resolve the 'Streaming is not supported in sync OpenAIAdapter' error") |
There was a problem hiding this comment.
Validation script does not fail on regression.
This script prints failure markers but still exits successfully, so it can report “All tests passed!” even when defaults are wrong.
Suggested fix
@@
- if stream_param and stream_param.default == False:
+ failures = []
+
+ if stream_param is not None and stream_param.default is False:
print("✓ _chat_completion method has stream=False as default")
else:
- print(f"✗ _chat_completion method stream default is: {stream_param.default}")
+ actual = getattr(stream_param, "default", "<missing parameter>")
+ failures.append(f"_chat_completion stream default is {actual!r} (expected False)")
@@
- if unified_stream_param and unified_stream_param.default == False:
+ if unified_stream_param is not None and unified_stream_param.default is False:
print("✓ _execute_unified_chat_completion method has stream=False as default")
else:
- print(f"✗ _execute_unified_chat_completion method stream default is: {unified_stream_param.default}")
+ actual = getattr(unified_stream_param, "default", "<missing parameter>")
+ failures.append(
+ f"_execute_unified_chat_completion stream default is {actual!r} (expected False)"
+ )
+
+ if failures:
+ for failure in failures:
+ print(f"✗ {failure}")
+ sys.exit(1)
@@
-print("✓ All tests passed!")
+print("✓ All tests passed!")Also applies to: 51-51
🧰 Tools
🪛 Ruff (0.15.13)
[error] 29-29: Avoid equality comparisons to False; use not stream_param.default: for false checks
Replace with not stream_param.default
(E712)
[error] 37-37: Avoid equality comparisons to False; use not unified_stream_param.default: for false checks
Replace with not unified_stream_param.default
(E712)
🤖 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 `@test_multiagent_fix.py` around lines 29 - 43, The validation script currently
prints pass/fail messages but always exits zero; update the checks for
_chat_completion and ChatMixin._execute_unified_chat_completion so that when
either stream default is not False the script records failure and exits with a
non-zero status (e.g., call sys.exit(1) or set a failure flag and exit after
checks). Locate the inspection logic around symbols _chat_completion and
ChatMixin._execute_unified_chat_completion in test_multiagent_fix.py (also the
similar check around line 51) and ensure any failure path triggers a non-zero
exit so CI will fail on regressions.
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/c4a541cc-a3dc-4637-85d9-53f7090dcb35 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/c4a541cc-a3dc-4637-85d9-53f7090dcb35 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Reviewed all available reviewer outputs first: Qodo had no review (paused), CodeRabbit had no functional findings to address, and there were no Gemini review comments on this PR. I found one valid gap and fixed it in Review summary:
If you want, I can also add one integration-level regression test for multi-agent sync execution path specifically. |
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review ✅Phase 1: Review per AGENTS.md
Phase 2: Fix Valid Issues
Phase 3: Final Verdict
✅ APPROVED - Ready to MergeArchitecture Compliance ✅This PR follows AGENTS.md principles:
Code Quality ✅
Issues Addressed ✅
Files Modified:
What Was Skipped:
This PR correctly fixes the streaming incompatibility issue while maintaining architectural integrity. The changes are minimal, well-tested, and properly documented. |
Addresses reviewer feedback about intentional difference between: - sync methods: stream=False (sync adapters don't support streaming) - async methods: stream=True (async adapters do support streaming) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
Fixes #1733
Summary
Changes
Testing
🤖 Generated with Claude Code
Summary by CodeRabbit