-
Notifications
You must be signed in to change notification settings - Fork 628
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
types: enable passing messages with arbitrary role #462
Conversation
… to enable granite3.2 thinking control functionality
This PR addresses the original issue you reported within the ollama-python client. I was able to reproduce your original issue with the original code that you shared, and it does not use langchain. The new traceback you shared above is different and is a separate error coming from langchain-core, which is a different project/codebase. Will you please share the code or command that you ran that resulted in the langchain error above? I'm happy to take a look and try to open a PR for langchain-core as well. |
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.
One quick addition if we're going to go the route of expanding the set of literals, we should consider including "document"
as well.
Co-authored-by: Gabe Goodhart <[email protected]>
Thanks, this is my orighinal post with original code : |
Hey @rylativity thanks for this PR. I think we're gonna remove the validation instead of hardcoding all the possible values. This is expected to grow with various training and post-training. Will close on merge of that |
@ParthSareen I'd prefer that approach as well. I'm happy to open a PR (or modify this one) to remove the validation entirely unless there is already a PR opened for it. |
I don't think there is yet. You're welcome to take a crack at it. Maybe we can start with the Ollama repo and continue from there. Thanks! |
@ParthSareen I've updated this PR to allow passing arbitrary roles through ollama-python rather than checking for a value in a specific set of literals. Ollama server already appears to allow arbitrary roles to be passed and processed. The only area where it is validated and prevented is in the Modelfile parsing. I've opened a PR to address the Modelfile parsing here - ollama/ollama#9874 Here's an example curl request to an ollama server instance without any modifications showing that it doesn't validate or block roles:
|
@ParthSareen formatting and linting issues reported by ruff should now be resolved for whenever you want to rerun the testing/linting 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.
Thanks for updating the tests with the change as well :)
This reverts commit 07eec6d.
Ollama's python client currently only permits messages with roles of either 'user', 'assistant', 'system', or 'tool'.
As described here - ollama/ollama#8955 (comment) - this prevents sending messages with the "control" role, which was introduced by IBM's granite3.2 to enable turning the "thinking" functionality on and off as needed.
This pull request adds "control" to the list of valid values for a message's "role" field/attribute so that these messages can be passed through to ollama.
Note: ollama server already handles messages with this role appropriately, but the python client currently does not allow sending them to ollama server. This resolves the issue in the python client.