-
Notifications
You must be signed in to change notification settings - Fork 15
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
405 abstract ModelType - inline ModelType #477
Conversation
# Conflicts: # gpt4all-kotlin/src/jvmMain/kotlin/com/xebia/functional/gpt4all/GPT4All.kt # gpt4all-kotlin/src/jvmMain/kotlin/com/xebia/functional/gpt4all/HuggingFaceLocalEmbeddings.kt # integrations/gcp/src/commonMain/kotlin/com/xebia/functional/xef/gcp/GCP.kt # integrations/gcp/src/commonMain/kotlin/com/xebia/functional/xef/gcp/models/GcpChat.kt # openai/src/commonMain/kotlin/com/xebia/functional/xef/conversation/llm/openai/OpenAI.kt # tokenizer/src/commonMain/kotlin/com/xebia/functional/tokenizer/ModelType.kt
# Conflicts: # gpt4all-kotlin/src/jvmMain/kotlin/com/xebia/functional/gpt4all/GPT4All.kt # gpt4all-kotlin/src/jvmMain/kotlin/com/xebia/functional/gpt4all/HuggingFaceLocalEmbeddings.kt # integrations/gcp/src/commonMain/kotlin/com/xebia/functional/xef/gcp/GCP.kt # integrations/sql/src/main/kotlin/com/xebia/functional/xef/sql/SQL.kt # openai/src/commonMain/kotlin/com/xebia/functional/xef/conversation/llm/openai/OpenAI.kt
(contextLength as? MaxIoContextLength.Combined)?.total | ||
?: error( | ||
"accessing maxContextLength requires model's context length to be of type MaxIoContextLength.Combined" | ||
) |
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.
As a side note, again - this is supposed to be an intermediary solution. Usages of this field that use an OAI model will still work as before. If this field is called on an instance of a (Google) model that doesn't use the combined context length an exception imminent.
I found this to be the best way to handle it, in favor to not change too much code in one PR.
Hey everyone 👋, I think this PR is ready for a first review. In the very infancy of this PR i discussed my ideas with @raulraja. This is what I came up with. Open for discussion :) |
Hi Ron, we are getting rid of the GCP integrations in favor of having a single server compatible with the Open AI YAML spec based on the branch work in |
Vale.. 🥺 I was fighting so hard to get GCP to the same support level as OAI. 😪 |
The idea of this PR is outlined in #405 .
In short, ModelType is currently too much designed for the OpenAI. By inlining all the properties and functionality of ModelType to the right interface in the LLM hierarchy, it can be made more generic and type safe.
What I did here:
ModelID
value class (kinda as a replacement forModelType
as this is the only feature all LLMs shareOpenAIModel
MaxIoContextLength
- OpenAI's models all have a shared limit for input and output tokens combined, some of Google's models have separate limits (one for input and output)tokensFromMessages
had to be moved out ofLLM
; additionallycountTokens
andtruncateText
have moved to the same subclass ofLLM
modelType
Besides the internal refactoring, tests and integrations had to be adapted.
TokenTextSplitter
now takesEncodingType
rather thanModelType
as parameter (as is still specific to OAI)If this is approved, a subsequent PR building on these changes may be considered to abstract the encoding part for estimating the tokens. Part of this job is adapting internal APIs and integrations to the new more abstract
contextLength
(of typeMaxIoContextLength
).