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.

Sign up for GitHub

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

Jump to bottom

405 abstract ModelType - inline ModelType #477

Closed
wants to merge 22 commits into from
Closed

405 abstract ModelType - inline ModelType #477

wants to merge 22 commits into from

Conversation

Intex32
Copy link
Member

@Intex32 Intex32 commented Oct 4, 2023 •
edited
Loading

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:

  • introduces a ModelID value class (kinda as a replacement for ModelType as this is the only feature all LLMs share
  • inlines the model's name and context length - and the encoding for all OpenAI models into new interface OpenAIModel
  • introduce 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)
  • the OpenAI specific implementation of tokensFromMessages had to be moved out of LLM; additionally countTokens and truncateText have moved to the same subclass of LLM
  • changes the Tokenizer functionaliy to use Encoding directly rather than through the model's modelType

Besides the internal refactoring, tests and integrations had to be adapted.

  • TokenTextSplitter now takes EncodingType rather than ModelType as parameter (as is still specific to OAI)
  • some tests that were using the encoding type or context length had to be adapted

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 type MaxIoContextLength).

@Intex32
cherrypicked most changes from former branch
ebb0883
@Intex32
Merge branch 'main' into 405-abstract-modeltype-lento
9e71796 
# 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
@Intex32
one step closer to make code compiling
dc426c3
@Intex32
one step closer to make code compiling
004f980
@Intex32
delete ModelType.kt
eea22e8
@Intex32
rename MaxContextLength.kt
3441197
@Intex32 Intex32 linked an issue Oct 4, 2023 that may be closed by this pull request
abstract ModelType #405
Open
@Intex32 Intex32 self-assigned this Oct 4, 2023
@Intex32 Intex32 added help wanted Extra attention is needed Core labels Oct 4, 2023
Intex32 and others added 13 commits October 4, 2023 15:38
@Intex32
spotless
aea81bb
@Intex32
first compiling version (no clean build)
a4eef4b
@Intex32
spotless
3aa8be6
@Intex32
pls compile compadre
a267c88
@Intex32
small changes
f0075a4
@Intex32
small changes
9f23295
@Intex32
Merge branch 'main' into 405-abstract-modeltype-lento
736912f 
# 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
@Intex32
fixed stuff after merge
a251225
@Intex32 @github-actions
Apply spotless formatting
96fe57c
@Intex32
many small changes; introduces BaseChat
7a628e0
@Intex32 @github-actions
Apply spotless formatting
c6b56e2
@Intex32
removes some intermediary functions in LLM and adapts all usages
1f19f61
@Intex32
fix potential bug with integer overflow in PromptCalculator
253adba
@Intex32 Intex32 removed the help wanted Extra attention is needed label Nov 25, 2023
Intex32
Intex32 commented Nov 25, 2023
View reviewed changes
core/src/commonMain/kotlin/com/xebia/functional/xef/llm/BaseChat.kt
(contextLength as? MaxIoContextLength.Combined)?.total
?: error(
"accessing maxContextLength requires model's context length to be of type MaxIoContextLength.Combined"
)
Copy link
Member Author

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.

@Intex32 Intex32 marked this pull request as ready for review November 25, 2023 21:55
@Intex32
Copy link
Member Author

Intex32 commented Nov 25, 2023

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 :)
Since I have no GCP token anymore, somebody pls test if the GCP example is still working.

@raulraja
Copy link
Contributor

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 use-generated-openai-client. That branch actually gets rid of the entire LLM hierarchy and autogenerates the services based on the YAML spec.

@Intex32
Copy link
Member Author

Intex32 commented Nov 27, 2023 •
edited
Loading

Hi Ron, we are getting rid of the GCP integrations

Vale.. 🥺 I was fighting so hard to get GCP to the same support level as OAI. 😪
You changed so much again and again in Arrow until you reached 1.0. I guess that's normal.

@javipacheco javipacheco closed this Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Intex32 Intex32

Labels
Core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

abstract ModelType
3 participants
@Intex32 @raulraja @javipacheco