-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: Support Multiple System Messages with Better API #3167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @OmCheeLin 's contribution! based on previous discussion: #3113 (comment) I think we also need to update ScoreBasedContextCreator to fully support multiple system message setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @OmCheeLin
To make the PR comprehensive, these missing parts also need to be addressed:
- adding test cases
- the method add_or_update_system_messages doesn't seem to implement the multiple system messages we are looking for. It must be able to create more than one system message, even in the middle of the conversation.
|
hi @OmCheeLin , Hope you're doing well! Just wanted to check in on this PR. It looks like there's some feedback that needs to be addressed, along with some failing in pre-commit checks. Please let us know if you have any questions or need a hand with anything. We're looking forward to move this feature forward with you! Cheers |
|
I have asked hesamsheikh in the issue before, but he hasn't replied to me yet. Of course, this issue can be given to others if anyone wants to do it. |
|
Hey @OmCheeLin I haven't seen any questions asked here or on Slack. Have I missed anything? I have already given a couple feedbacks, and you removed the "review required" tag. If you need assistance or clarifications, let me know. Thank you. |
oh, I asked in #3141, I will send messages under pr in the future
|
|
Thanks for pointing it out @OmCheeLin |
|
Hey @OmCheeLin Following the discussions in the issue #3141 , let's now focus on making the API more flexible and intuitive and then later on, work on having multiple system messages. I think the best approach for now would be to imrprove the following areas:
For now this is more needed and also an easier to implement enhancement than the multi system messages. To be honest, I'm not even sure we would need to have multiple system messages if we have the |
|
@hesamsheikh I agree with your point of view. If @Wendong-Fan agrees as well, I can start working. |
thanks @OmCheeLin and @hesamsheikh ! Let's add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fast implementation @OmCheeLin
I added a few comments. I'm not sure if it is necessary to reset the memory after appending to or updating the system message. At that point, maybe it makes more sense to initialize the agent again? Wdyt?
camel/agents/chat_agent.py
Outdated
| if self._original_system_message | ||
| else "" | ||
| ) | ||
| new_system_message = original_content + content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's put some kind of a separator between the existing and the new system message. like a newline.
| Can be either a BaseMessage object or a string. | ||
| If a string is provided, it will be converted | ||
| into a BaseMessage object. | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a validation for case that None is passed (like return)
Resetting the entire system means clearing the output format, language, tools and etc. Do I need to do this? |
What I mean is, you reset the memory after updating or appending to the system message. let's add an argument to those methods to make it customizable whether the memory must be reset or just continue the conversation. |

Description
Support Multiple System Messages with Better API. #3141
Checklist
Go over all the following points, and put an
xin all the boxes that apply.Fixes #issue-numberin the PR description (required)pyproject.tomlanduv lockIf you are unsure about any of these, don't hesitate to ask. We are here to help!