Skip to content
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

Ollama chat endpoint support #16

Merged
merged 12 commits into from
Feb 7, 2024

Conversation

tquartus
Copy link
Contributor

Hello, Andrew,

First off, thanks for your project! I wrote some code over a year ago for interacting with the OpenAI API via Emacs. OpenAI broke that several times with their changes. Recently, I started working with LLMs locally and decided to rewrite everything from scratch to support multiple APIs ... and discovered that, thankfully, someone had beaten me to that! :-)

I noticed that the existing package provided support only for the Ollama /generate endpoint, but not the /chat endpoint. I find the /generate endpoint simpler for one-off requests. But for chat conversations, with multiple interactions, I prefer the /chat endpoint. For that, I can generate a set of interactions from scratch, regenerate responses, or switch between different LLMs in the same conversation. So, I've introduced some changes to support the other endpoint. I believe it's backward compatible.

The main changes include:

  1. A new :endpoint slot in the llm-ollama structure, which specifies the API endpoint to use. This slot defaults to generate, but can be set to chat to utilize the /chat endpoint.

  2. Adjustments to the JSON encodings for making requests to the API and processing responses from the server. These changes accommodate the differences between the /generate and /chat endpoints. Specifically, separate helper functions have been created to handle the request data for each endpoint.

  3. Modifications to the llm-ollama--chat-request function to call the appropriate helper function based on the specified endpoint.

  4. Updates to the llm-ollama--url calls in the llm-chat and llm-chat-streaming methods to use the specified endpoint.

  5. Updated documentation to reflect the new :endpoint slot in the llm-ollama structure.

Hopefully, support for both endpoints aligns with the package's goal of abstracting functionality to a higher level and concealing API variations.

I look forward to your feedback and the opportunity to contribute to the ongoing development of this package.

Best,
Thomas

@ahyatt
Copy link
Owner

ahyatt commented Jan 14, 2024

Thanks for this change! Before I take a look, do you have FSF copyright assignment? This is needed to contribute substantial changes to this project, since it is part of GNU ELPA.

@tquartus
Copy link
Contributor Author

In process. Emailed FSF just as I sent this in. Hopefully, won't take long. I'm a college professor, but our faculty handbook makes it clear they don't claim copyright over works by faculty. Hoping FSF will accept that without requiring additional signatures. Sorry for the delay. Will let you know when it's settled.

@ahyatt
Copy link
Owner

ahyatt commented Jan 14, 2024

No problem, thank you for getting that process started! And without looking at your code, I'd suggest that it's OK to go to the chat endpoint by default. That is typically what we do for other providers.

@tquartus
Copy link
Contributor Author

tquartus commented Feb 4, 2024

My FSF copyright assignment finally came through. Just wanted to let you know.

Copy link
Owner

@ahyatt ahyatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for these changes and for getting the copyright assignment!

llm-ollama.el Outdated
`request-alist' depending on whether a `chat' or `generate'
endpoint is specified in the provider."
(let* ((request-alist
(if (string= (llm-ollama-endpoint provider) "chat")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just use "equal" here, string= is more about comparing without properties.

README.org Outdated
@@ -56,6 +56,7 @@ In addition to the provider, which you may want multiple of (for example, to cha
- ~:port~: The port that ollama is run on. This is optional and will default to the default ollama port.
- ~:chat-model~: The model name to use for chat. This is not optional for chat use, since there is no default.
- ~:embedding-model~: The model name to use for embeddings. This is not optional for embedding use, since there is no default.
- ~:endpoint~: The ollama endpoint to use, either "generate" or "chat". This is optional and will default to "generate".
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it makes a difference to use "generate", when we could just use "chat" always? If there's no appreciable difference, I'd prefer not having an option.

llm-ollama.el Outdated
@@ -178,7 +220,7 @@ STREAMING if non-nil, turn on response streaming."
;; we really just need it for the local variables.
(with-temp-buffer
(let ((output (llm-request-sync-raw-output
(llm-ollama--url provider "generate")
(llm-ollama--url provider (slot-value provider 'endpoint))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but stylistically the rest of the code prefers to use (llm-ollama-endpoint provider), so can you change this and the other instance of this (assuming you keep the the endpoint slot?)

@tquartus
Copy link
Contributor Author

tquartus commented Feb 7, 2024

Thanks! Based on your suggestions, I've removed support for the /generate endpoint and now only support the more versatile /chat endpoint. This simplifies the code and makes it more straightforward. As a result, the code involving string= and the :endpoint slot became unnecessary, so I've removed them as well, and also removed the added line from the README.org. Please let me know if there's anything else you'd like me to adjust.

@ahyatt ahyatt merged commit a343797 into ahyatt:main Feb 7, 2024
@ahyatt
Copy link
Owner

ahyatt commented Feb 7, 2024

Thank you for your change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants