Updated message trim to handle message better and support tool calling. - #28
Conversation
c0b3259 to
2061b20
Compare
|
Dette er et potentielt fiks til os2ai/Feedback#1. |
|
@lasseborly vil du lave code review? Vi kører den udenom den igangværende RC som et hot fix og ser om det kan være med til at løse Holstebros udfordring |
lasseborly
left a comment
There was a problem hiding this comment.
I have a hard time wrapping my head around the actual logic and how it solves the apparent problems. Not that it does not, I am just very removed from the problem space.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class MessageTrimmingGuardrail(CustomGuardrail): |
There was a problem hiding this comment.
I think I'm in doubt as to what the purpose of the message trimming guardrail is. Is there a reason we are trimming messages at all? It seems like trying to trim them is what causes the behavior users are reporting back as bad, but I am not sure I understand the motivation for having trimming in the first place?
There was a problem hiding this comment.
@cableman We talked about adding a short comment with a link from here to some documentation. In the documentation we would like to document:
- Reasons for adding this guardrail: models that run out of context window.
- Principles in message trimming: how do we approach the problem, how do we weigh options.
There was a problem hiding this comment.
@hypesystem create a new issue regarding how to handle documentation in the project, it should not be decided on a case to case basis. For now read the documentation here: os2ai/documentation#6
There was a problem hiding this comment.
@lilosti I'm not sure exactly what this means or what outcome you would like from it, but we're happy to improve general processes where it makes sense! Let's try and talk about it in person to land it at some point 😄
hypesystem
left a comment
There was a problem hiding this comment.
The code makes sense when considering the documentation 😄
I still think a one-liner on why this code exists would make sense, if not a link to external documentation.
I added a few small notes on validation and a single one on less significant logic. Nothing blocking, fine to merge with or without fixes.
17b6a67 to
a3d81f9
Compare
hypesystem
left a comment
There was a problem hiding this comment.
A beaut, I declare! Good work 😄
| type(value).__name__, | ||
| ) | ||
| return {} | ||
| return value |
See https://github.com/AarhusAI/aarhusai-docker/blob/main/guardrails/README.md for more information about the message trim guardrail.
New configuration knobs
New tool-calling repair logic
Behavior changes