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

[ENH] Add support for Ollama assistants #376

Merged

Conversation

smokestacklightnin
Copy link
Contributor

@smokestacklightnin smokestacklightnin commented Mar 23, 2024

This PR adds support for Ollama assistants.

ragna/assistants/_ollama.py Outdated Show resolved Hide resolved
ragna/assistants/_ollama.py Outdated Show resolved Hide resolved
ragna/assistants/_ollama.py Outdated Show resolved Hide resolved
ragna/assistants/_ollama.py Outdated Show resolved Hide resolved
ragna/assistants/_api.py Outdated Show resolved Hide resolved
@smokestacklightnin smokestacklightnin force-pushed the assistants/ollama/basic-functionality branch from 85030be to fd5c34b Compare April 10, 2024 06:41
@smokestacklightnin smokestacklightnin marked this pull request as ready for review April 12, 2024 00:41
Also fix typo in `ragna.assistants.__init__`
Comment on lines 93 to 96
if "error" in json_data:
raise RagnaException(json_data["error"])
if not json_data["done"]:
yield cast(str, json_data["message"]["content"])
Copy link
Member

@pmeier pmeier May 28, 2024

Choose a reason for hiding this comment

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

@nenb This violates "same response schema" part of #425 (comment) 😖

@pmeier pmeier requested a review from nenb May 30, 2024 18:30
@pmeier
Copy link
Member

pmeier commented May 30, 2024

@nenb As follow-up to #376 (comment), I've refactored the logic we merged in #425 a little to make it even more flexible.

  • We now have an HttpApiCaller base class that handles the different streaming protocols for us. It is instantiated by the base class of the builtin assistants and parametrized by the streaming protocol, which is set as class constant.
  • This new HttpApiCaller also handles JSON streaming for Google assistants now and thus abstracts the complexity away.
  • I've renamed the OpenaiCompliantHttpApiAssistant to OpenaiLikeHttpApiAssistant, because the term "compliant" is even less meaningless when adding Ollama into the mix
  • Each subclass of OpenaiLikeHttpApiAssistant now has to implement answer itself given that their is no standard way of handling it. The only common functionality is the call to the API, which so far seems to be rather constant across all models.

Copy link
Contributor

@nenb nenb left a comment

Choose a reason for hiding this comment

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

This looks fine to me.

I've pulled the branch locally and confirmed that it works for OpenAI models and for a Llamafile that I had locally.

@pmeier pmeier merged commit da1bcc2 into Quansight:main Jun 10, 2024
10 checks passed
@pmeier pmeier deleted the assistants/ollama/basic-functionality branch June 10, 2024 09:25
blakerosenthal pushed a commit that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ApiAssistant abstract base class for assistants that don't require an API key
3 participants