-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Fix message history limiter for tool call #3178
Fix message history limiter for tool call #3178
Conversation
@microsoft-github-policy-service agree |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3178 +/- ##
===========================================
- Coverage 32.90% 19.79% -13.12%
===========================================
Files 94 103 +9
Lines 10235 10913 +678
Branches 2193 2491 +298
===========================================
- Hits 3368 2160 -1208
- Misses 6580 8547 +1967
+ Partials 287 206 -81
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks @GaoxiangLuo for putting this together and for fixing this error. I've tested your updated transform code and it does work. However, in testing I have identified that by truncating the messages more (e.g. I'm proposing a change to the MessageHistoryLimiter that now includes an additional parameter called Let me know what you think. Change class MessageHistoryLimiter's
and the
Let me know what you think and whether you want to include it in this change, otherwise I'll create a new PR. |
Thank you @marklysze. LGTM. Please include it in this change. |
Great, thanks @GaoxiangLuo, I've updated the branch to include those changes. It would worth testing and let us know how you go :). |
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.
Thank you @GaoxiangLuo @marklysze for these fixes! I'm just worried that the total count of messages, after applying this transform, becomes unintuitive. For example, if I set max_messages = 5, keep_first_message = True
, intuitively I would think I would have a list of 5 messages with the first being the first created message. But with this fix, I believe I get back 6 messages.
What do you think of using a queue and just anchoring the important messages (first message, tool call etc..) to respect themax_messages
argument? Everything else looks great and I'll let you guys decide on the final solution!
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.
Looks good!
Thanks @WaelKarkoub! |
CC @qingyun-wu, @sonichi - I think we're good to go on this one. |
* fix: message history limiter to support tool calls * add: pytest and docs for message history limiter for tool calls * Added keep_first_message for HistoryLimiter transform * Update to inbetween to between * Updated keep_first_message to non-optional, logic for history limiter * Update transforms.py * Update test_transforms to match utils introduction, add keep_first_message testing * Update test_transforms.py for pre-commit checks --------- Co-authored-by: Mark Sze <[email protected]> Co-authored-by: Chi Wang <[email protected]>
Why are these changes needed?
It will return an error
because OpenAI API has to takes in pairs of
tool_calls
andtool
.The following is a minimal example to reproduce the error.
Related issue number
#3121
Checks