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

ApiAssistant abstract base class for assistants that don't require an API key #375

Open
smokestacklightnin opened this issue Mar 22, 2024 · 2 comments
Labels
type: enhancement 💅 New feature or request

Comments

@smokestacklightnin
Copy link
Contributor

smokestacklightnin commented Mar 22, 2024

Feature description

Previously, we only had assistants hosted nonlocally. Now, adding Ollama, we will be supporting local assistants. The remote assistants all required API keys, but local ones like Ollama do not. That is really the only substantial difference; Ollama still is accessed via a REST API. Does it make sense to others to refactor ragna.assistants._api.ApiAssistant to not require an API key and then subclass it with the newly created subclass requiring the key?

An alternative is subclassing ragna.core.Assistant from "scratch" and not requiring an API key. This option seems suboptimal to me because all the behavior is otherwise the same as ragna.assistants._api.ApiAssistant.

Another alternative is to override the requirements to remove the API key requirement.

Value and/or benefit

No response

Anything else?

No response

@smokestacklightnin smokestacklightnin added the type: enhancement 💅 New feature or request label Mar 22, 2024
@smokestacklightnin
Copy link
Contributor Author

@pmeier Do you have an opinion?

@pmeier
Copy link
Member

pmeier commented Mar 25, 2024

Now, adding Ollama, we will be supporting local assistants

To give some context here: this is not decided yet. We are exploring this an option.

Does it make sense to others to refactor ragna.assistants._api.ApiAssistant to not require an API key and then subclass it with the newly created subclass requiring the key?

So far the new Ollama assistant is the only outlier. Thus, I wouldn't break our abstraction just yet, given that a workaround is available (see below).

In case we encounter 1 or 2 more exceptions (maybe vLLM) to our current rule, I indeed agree it would make sense to refactor the class. I would move the API key functionality to a class AuthorizedApiAssistant(ApiAssistant) (name TBD) subclass.

An alternative is subclassing ragna.core.Assistant from "scratch" and not requiring an API key. This option seems suboptimal to me because all the behavior is otherwise the same as ragna.assistants._api.ApiAssistant.

Another alternative is to override the requirements to remove the API key requirement.

Of those two, I would vote for the latter. That is a simple enough workaround that can easily be removed if we go for the more general solution explained above in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement 💅 New feature or request
Projects
None yet
2 participants