-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add support for the developer role in chat completion messages #117
Conversation
Signed-off-by: Ignasi Barrera <[email protected]>
thanks @nacx ! Can you help add a test? |
What kind of additional test do you mean? I already modified the unit test that unmarshals the request to make sure it it properly handled, so I think that should be enough for the scope of the change? I mean, by adding, say, an e2e test we would be jsu testing that the backend understands that |
How does it translated into Bedrock request ? In OpenAIMessageToBedrockMessage, if the role is |
fyi this is a bug in ci setting - i am working on it |
i think the existing unit test should be good. Now the question is how to translate this in aws bedrock request. Erring seems reasonable though |
leaving it as a TODO with a new issue would be good as well |
@nacx can you merge the main? I think we could merge as-is |
This is a follow-up PR for: #117 (comment) I think it makes sense to do the same conversions and set it as "system"? The "developer" role is a replacement for "system" in OpenAI, so it should be fine? Signed-off-by: Ignasi Barrera <[email protected]>
This is a follow-up PR for: #117 (comment) I think it makes sense to do the same conversions and set it as "system"? The "developer" role is a replacement for "system" in OpenAI, so it should be fine? Signed-off-by: Ignasi Barrera <[email protected]>
With o1 models and newer,
developer
messages replace the previoussystem
messages.