feat: allow components to declare their chat role via format_for_llm - #1542
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
| if tr.role is not None: | ||
| role = cast(Message.Role, tr.role) |
There was a problem hiding this comment.
Is there a reason to cast this here? If role is set to a random str (ie outside the literals defined in Message.Role), cast won't actually change its type.
I think there are two questions here:
- should we allow open ended / arbitrary roles?
- do we force Messages to comply with Message.Role anywhere?
There was a problem hiding this comment.
Re arbitrary roles - while we could add more I think the existing set is 1:1 with the backends.
Re complying - yes in Message.init - so should be able to remove the cast here without issue.
jakelorocco
left a comment
jakelorocco
left a comment
There was a problem hiding this comment.
Sorry; took another look at some of the implications and found a few:
- as_chat_history and as_generic_chat_history don't respect these new role fields
- a component with a tool role can't declare all the fields necessary for a ToolMessage (ie tool call id); this is an odd case but seems necessary to adress
| case Message(): | ||
| return c |
There was a problem hiding this comment.
I wonder if we should fold Message into the regular component case now? And ensure it's template repr gets the proper role?
There was a problem hiding this comment.
Took a stab at this, but I think it's more of a halfway compromise.
From my current understanding if we move message entirely from Message to Component the ToolMessage work done in #1389 would regress. May be worth a further discussion / design.
There was a problem hiding this comment.
Makes sense. I'm fine with postponing some sort of collapsing of special Message handling with Component handling. Can you please open an issue for it explaining the issues you found as well?
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
| case Message(): | ||
| return c |
There was a problem hiding this comment.
Makes sense. I'm fine with postponing some sort of collapsing of special Message handling with Component handling. Can you please open an issue for it explaining the issues you found as well?
|
Will open the followup Mon so I can dive a bit deeper into how we might work around the issue I ran into doing the direction conversion. |
949d74e
2 participants
Pull Request
Issue
Fixes #377
Description
allow components to declare their chat role via format_for_llm
Needed as a precursor to #1030
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.