-
-
Notifications
You must be signed in to change notification settings - Fork 232
Anthropic: Fix system prompt (use plain text instead of serialized JSON) #302
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
Conversation
…ON instead of pure string, make temperature optional
Just a note that #234 also fixes this issue but in a slightly different way. https://docs.anthropic.com/en/api/messages#body-system shows that you can provide a string or an array of objects. When caching system prompts, you have to use the array to provide the |
Thanks @tpaulshippy ! If #234 solves the issue and is merged soon, it's good to me 🙂 I noticed in the cassettes that the JSON was not serialized anymore so that should be ok. But since it's a bug, if #234 still need some time to be merged, this one could maybe be merged first? There will not be a lot of conflicts. And I still think that the |
Agreed. Would love to see this get merged ASAP. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #302 +/- ##
=======================================
Coverage 86.61% 86.61%
=======================================
Files 79 79
Lines 3123 3124 +1
Branches 611 612 +1
=======================================
+ Hits 2705 2706 +1
Misses 418 418 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
LGTM
…ON) (crmne#302) ## What this does When migrating from [ruby-openai](https://github.com/alexrudall/ruby-openai), I had some issues getting the same responses in my Anthropic test suite. After some digging, I observed that the Anthropic requests send the `system context` as serialized JSON instead of a plain string like described in the [API reference](https://docs.anthropic.com/en/api/messages#body-system): ```ruby { :system => "{type:\n \"text\", text: \"You must include the exact phrase \\\"XKCD7392\\\" somewhere\n in your response.\"}", [...] } ``` instead of : ```ruby { :system => "You must include the exact phrase \"XKCD7392\" somewhere in your response.", [...] } ``` It works quite well (the model still understands it) but it uses more tokens than needed. It could also mislead the model in interpreting the system prompt. This PR fixed it. I also took the initiative to make the temperature an optional parameter ([just like with OpenAI](https://github.com/crmne/ruby_llm/blob/main/lib/ruby_llm/providers/openai/chat.rb#L21-L22)). I hope it's not too much for a single PR, but since I was already re-recording the cassettes, I figured it would be easier. I'm sorry but I don't have any API key for Bedrock/OpenRouter. I only recorded the main Anthropic cassettes. ## Type of change - [x] Bug fix - [ ] New feature - [ ] Breaking change - [ ] Documentation - [ ] Performance improvement ## Scope check - [x] I read the [Contributing Guide](https://github.com/crmne/ruby_llm/blob/main/CONTRIBUTING.md) - [x] This aligns with RubyLLM's focus on **LLM communication** - [x] This isn't application-specific logic that belongs in user code - [x] This benefits most users, not just my specific use case ## Quality check - [x] I ran `overcommit --install` and all hooks pass - [x] I tested my changes thoroughly - [ ] I updated documentation if needed - [x] I didn't modify auto-generated files manually (`models.json`, `aliases.json`) ## API changes - [ ] Breaking change - [ ] New public methods/classes - [ ] Changed method signatures - [ ] No API changes ## Related issues <!-- Link issues: "Fixes crmne#123" or "Related to crmne#123" --> --------- Co-authored-by: Carmine Paolino <[email protected]>
What this does
When migrating from ruby-openai, I had some issues getting the same responses in my Anthropic test suite.
After some digging, I observed that the Anthropic requests send the
system context
as serialized JSON instead of a plain string like described in the API reference:instead of :
It works quite well (the model still understands it) but it uses more tokens than needed. It could also mislead the model in interpreting the system prompt.
This PR fixed it. I also took the initiative to make the temperature an optional parameter (just like with OpenAI). I hope it's not too much for a single PR, but since I was already re-recording the cassettes, I figured it would be easier.
I'm sorry but I don't have any API key for Bedrock/OpenRouter. I only recorded the main Anthropic cassettes.
Type of change
Scope check
Quality check
overcommit --install
and all hooks passmodels.json
,aliases.json
)API changes
Related issues