Refactor ApiAssistant to AuthenticatedApiAssistant and UnauthenticatedApiAssistant
#381
Conversation
There was a problem hiding this comment.
We misunderstood each other when spoke offline yesterday. What I meant is that we should simply not subclass the OllamaAssistant from the ApiAssistant for now. My original reasoning from #375 holds. I don't want to introduce another abstraction just for a single assistant. We can revisit this later. Until then, I'll close this PR.
| self._api_key = os.environ[self._API_KEY_ENV_VAR] | ||
|
|
||
|
|
||
| class UnauthenticatedApiAssistant(_ApiAssistant): |
There was a problem hiding this comment.
This class is not needed. We can simply use the superclass for this.
| @classmethod | ||
| def requirements(cls) -> list[Requirement]: | ||
| return [EnvVarRequirement(cls._API_KEY_ENV_VAR), *cls._extra_requirements()] | ||
| return [] | ||
|
|
||
| @classmethod | ||
| def _extra_requirements(cls) -> list[Requirement]: |
There was a problem hiding this comment.
Not sure if it makes sense to have this on the superclass if for now only the authenticated subclass needs it.
| @classmethod | ||
| def requirements(cls) -> list[Requirement]: | ||
| return [EnvVarRequirement(cls._API_KEY_ENV_VAR), *cls._extra_requirements()] | ||
| return [] |
There was a problem hiding this comment.
This is the default return if we don't override the method. Meaning, we can just leave it out here.
| and issubclass(assistant, _ApiAssistant) | ||
| and assistant is not _ApiAssistant | ||
| and assistant is not AuthenticatedApiAssistant | ||
| and assistant is not UnauthenticatedApiAssistant |
There was a problem hiding this comment.
| and issubclass(assistant, _ApiAssistant) | |
| and assistant is not _ApiAssistant | |
| and assistant is not AuthenticatedApiAssistant | |
| and assistant is not UnauthenticatedApiAssistant | |
| and issubclass(assistant, AuthenticatedApiAssistant) | |
| and assistant is not AuthenticatedApiAssistant |
In preparation for adding local assistants that do not require API keys, this PR refactors
ragna.assistants._api.ApiAssistant.This resolves Issue #375.
If anyone has strong opinions about the chosen naming conventions, please share them.
IMPORTANT: This PR should be merged after PR #380 is merged and before PR #376 is merged.