Revert "fix: prevent duplicate Task and Response display when using litellm with memory"#618
Conversation
…itellm with memory"
|
Caution Review failedThe pull request is closed. """ WalkthroughThis change removes the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Agent
participant LLM
User->>Agent: chat(prompt)
Agent->>LLM: get_response(prompt, verbose)
LLM-->>Agent: response
Agent->>Agent: Append to chat history, validate guardrails
Agent-->>User: response
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @MervinPraison, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. This PR is a direct revert of pull request #617, titled "fix: prevent duplicate Task and Response display when using litellm with memory". The original PR aimed to prevent duplicate display of LLM interactions by suppressing the display within the LLM layer and handling it explicitly in the agent layer. This revert removes that logic, restoring the display behavior to its state before PR #617 was merged.
Highlights
- Revert: This pull request fully reverts the changes introduced in PR #617.
- Display Logic: The changes remove the
suppress_displayparameter and the logic associated with preventing duplicate displays when using certain LLM configurations (specifically mentioned in the original PR title as litellm with memory).
Changelog
- src/praisonai-agents/praisonaiagents/agent/agent.py
- Removed the
suppress_display=Trueargument from thellm.get_responsecall (line 1112 in the original code). - Removed the subsequent
display_interactionblock (lines 1118-1121 in the original code) which was intended to handle display when it was suppressed in the LLM.
- Removed the
- src/praisonai-agents/praisonaiagents/llm/llm.py
- Removed the
suppress_display: bool = Falseparameter from theget_responsefunction signature (line 293 in the original code). - Removed the
and not suppress_displaycondition from multipleif verbose ...checks (lines 458, 466, 669, 677, 722, 745 in the original code) that controlled whendisplay_interactionwas called.
- Removed the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
There was a problem hiding this comment.
Code Review
This pull request reverts the changes made in PR #617, which aimed to prevent duplicate Task and Response displays when using litellm with memory. The revert involves removing the suppress_display parameter and its associated logic from agent.py and llm.py, and deleting the corresponding test file.
The revert seems mostly correct and addresses the removal of the suppression logic consistently. However, there's a potential issue in llm.py where the display logic simplification might lead to unintended displays when verbose is false.
It would also be beneficial for future context if the PR description included a brief rationale for why PR #617 is being reverted.
Summary of Findings
- Potential unintended display in
llm.py: Inllm.py, the simplification of conditional logic for displaying interactions (specifically, changingelif verbose ...toelse) might causedisplay_interactionto be called even whenverboseisFalse. This could lead to unexpected output. It's recommended to change theseelseconditions toelif verbose:. - Completeness of Revert: The removal of the
suppress_displayparameter and associated logic appears to be consistently applied acrossagent.pyandllm.py. The deletion oftest_duplicate_fix.pyis also appropriate for a revert.
Merge Readiness
The pull request correctly reverts the intended functionality. However, there are a couple of medium-severity issues identified in llm.py related to conditional display logic that could lead to display_interaction being called when verbose is False. It's recommended to address these issues before merging to ensure the display behavior is as expected. As an AI, I am not authorized to approve pull requests; please ensure this PR is reviewed and approved by a human maintainer.
| console=console | ||
| ) | ||
| elif verbose and not suppress_display: | ||
| else: |
There was a problem hiding this comment.
It looks like the change from elif verbose and not suppress_display: to a simple else: might lead to display_interaction being called even when verbose is False.
The original logic ensured that verbose was true for both branches involving display_interaction. With the current else, this branch will be taken if verbose and reasoning_content is false. This includes the case where verbose itself is False.
If display_interaction always produces output (and doesn't internally check verbose), this could lead to unexpected output when verbose is set to False.
Could we consider changing this else: to elif verbose: to maintain the explicit check for verbosity before calling display_interaction?
| else: | |
| elif verbose: |
| console=console | ||
| ) | ||
| elif verbose and not suppress_display: | ||
| else: |
There was a problem hiding this comment.
Similar to the comment above (around line 465), this else: condition might cause display_interaction to be called even when verbose is False.
The original elif verbose and not suppress_display: ensured verbose was true. The current else: will be triggered if verbose and reasoning_content is false, which includes scenarios where verbose itself is False.
To prevent unintended displays when verbose is False, would it be better to change this to elif verbose:?
| else: | |
| elif verbose: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ BugBot reviewed your changes and found no bugs!
BugBot free trial expires on June 11, 2025
You have used $0.00 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
|
@BugBot review this error |
…aude/issue-612-20250606_060924 Revert "fix: prevent duplicate Task and Response display when using litellm with memory"
1 participant
User description
Reverts #617
PR Type
Bug fix, Tests
Description
Reverts previous fix for duplicate Task and Response display.
Removes suppression of display logic in agent and LLM modules.
Deletes test file for duplicate display issue.
Changes walkthrough 📝
agent.py
Remove suppression of duplicate display in agent chatsrc/praisonai-agents/praisonaiagents/agent/agent.py
suppress_displayparameter from LLM call.interaction.
llm.py
Restore always-on display interaction in LLM responsessrc/praisonai-agents/praisonaiagents/llm/llm.py
suppress_displayparameter fromget_response.test_duplicate_fix.py
Remove duplicate display issue test scripttest_duplicate_fix.py
Summary by CodeRabbit