-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Enhance Ollama thinking parameter. Separate thinking content from final answer content output. #9020
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
base: main
Are you sure you want to change the base?
Enhance Ollama thinking parameter. Separate thinking content from final answer content output. #9020
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
// Quick test that string values work in practice | ||
const res = await ollamaHigh.invoke([ | ||
new HumanMessage({ content: "How many r in the word strawberry?" }) | ||
]); | ||
|
||
expect(res).toBeDefined(); | ||
expect(typeof res.content).toBe("string"); | ||
expect(res.content.length).toBeGreaterThan(0); |
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.
No reason to use the model if we just do type checks. In v1
branch we have migrated to Vitest which allows to create type tests.
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.
Hi @christian-bromann and thanks a lot for your review and feedback! :))
Can you please clarify what would be the best approach here in your opinion? As I already do test model invocation in other test (test string thinking parameter '$thinkLevel'
), I removed it from this one, as you suggested.
I read a bit about Vitest, and I guess we can do something like that here:
test("test type safety for thinking parameter values", () => {
expectTypeOf<ChatOllamaInput['think']>().toEqualTypeOf<boolean | ThinkingIntensity | undefined>()
expectTypeOf<'high'>().toExtend<ThinkingIntensity>()
expectTypeOf<'medium'>().toExtend<ThinkingIntensity>()
expectTypeOf<'low'>().toExtend<ThinkingIntensity>()
expectTypeOf<{ think: ThinkingIntensity }>().toExtend<Partial<ChatOllamaInput>>()
expectTypeOf<{ think: boolean }>().toExtend<Partial<ChatOllamaInput>>()
});
However, Vitest is still not present inside the package.json
file of langchain-ollama
package. I see that it's already used in some of your internal libraries and also you plan to migrate to it, but what would be the best option currently? On one hand, I am not sure if adding vitest
in the package.json would do any harm, on the other hand, if I try to do some workaround for type checking with Jest, this will create a technical debt for you in the future. What do you suggest?
Description of the feature
Updated
ChatOllamaInput
-thinking
property with the recent additions in Ollama Thinking. More specifically, thinking intensity (low,medium,high) was added. This change is already present in LangChain python library.Separated thinking content from the actual answer content. This was creating a big confusion for me initially, as I couldn't distinguish between thoughts/actual response. Ollama API provides
message.content
andmessage.thinking
attributes which were already consumed by LangChain JS. However they were combined into one token, which lead to the unified final content.In this PR,
thinking_content
attribute is added intoadditional_kwargs
which isolates the thinking content only. This attribute is available everytime thinking is enabled. Thecontent
attribute now holds the actual answer only.Results
Attaching an example
AIMessage
output payload I got while testing with local Ollamagpt-oss:20b
model:Tests
Comprehensive integration tests for this specific feature/s added
Docs
TODO...