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

Introduce UnifiedParameters #600

Merged
merged 37 commits into from
May 10, 2024

Conversation

codenamev
Copy link
Contributor

This is an initial effort to unify the API signature for all the LLMs' chat method. This follows OpenRouter's philosophy of "normalizes the schema across models and providers so you only need to learn one". Each LLM will need to implement their own adaptations of the base-unified parameter list, and follow some simple rules:

  1. All LLM subclasses will now use an instance of UnifiedParameters and can access it via chat_parameters
  2. All LLM subclasses can customize the schema used by the unified parameters in their initializer
  3. If the chosen model doesn't support a request parameter (such as logit_bias in non-OpenAI models, or top_k for OpenAI), then the parameter will be ignored. The rest will be forwarded to the underlying model API.

This is also an initial effort to set up the foundation for a Parameters API. The idea being that each model can use their customized UnifiedParameters as an interface for querying what those parameters are.

I'd love some feedback on this before I go spreading it to every other LLM class 🥺

@andreibondarev andreibondarev self-requested a review May 1, 2024 11:27
@andreibondarev
Copy link
Collaborator

@codenamev I should finish my review by the end of today.

tools: [],
tool_choice: nil,
user: nil,
&block
Copy link
Collaborator

Choose a reason for hiding this comment

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

@codenamev You didn't like how all of the params were listed out? We had that previously because I felt like it made the interface clearer since it's an interface that we control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was torn here, but allowing these inherited classes to change the method signature with custom keyword arguments complicates things quite a lot and is adding confusion on when to use what. One thing we could do is change this to accept a UnifiedParameters instance (instead of a Hash as it is) and then pass that along to the chat_parameters the same way (and just have it merge). Doing that would allow us to continue documenting and inspect what is acceptable criteria (similar to how GraphQL mutations can accept input types).

Copy link
Collaborator

@andreibondarev andreibondarev May 2, 2024

Choose a reason for hiding this comment

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

One thing we could do is change this to accept a UnifiedParameters instance

What would the DX look like then? Something along the lines of: ... ?

uniparams = Langchain::LLM::UnifiedParameters.new(schema: {}, parameters: params)
openai.chat(parameters: uniparams)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to re-iterate, the goal of these changes is that Langchain::LLM::Base is the source of truth for the unified method signature we want applied to all children. Preserving this idea, the LLM sub-classes will be responsible for:

  1. Adhering to the unified method signature of its' parent
  2. Managing any amendments to the method signature it wants to handle

The way this currently is implemented, each sub-class uses a chat_params instance from LLM::Base and modifies it in its' initializer.

Following 3 of OpenRouter's principles here:

If the chosen model doesn't support a request parameter, then the parameter will be ignored. The rest will be forwarded to the underlying model API.

The callers should be able to pass any parameters to these sub-classes and the model itself will know how to handle them for their respective API calls. For this reason, this is why I modified the arguments of chat for OpenAI to accept a hash rather than explicit keyword arguments. Under the hood, the model forces the unified schema, ignores any fields, re-maps (aliases) any fields it needs re-named, and allows any additional fields the model accepts.

In the future, we can add validations and parameter inquiries, but this is mostly a small step backward (obfuscating the method signature) in favor of applying the unified signature with backwards compatibility. I'll be adding further documentation for what these parameters are that I think will solve some of your worries here 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

@codenamev Okay, I love it, and I'm on board 🚢 🚀

Copy link
Collaborator

@andreibondarev andreibondarev May 6, 2024

Choose a reason for hiding this comment

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

(A proper docs website is desperately needed 😅) but we'd probably still want to catalogue all of the possible values that could be passed under the params={} hash here so that it shows up in the rubydoc as well.

@andreibondarev
Copy link
Collaborator

@codenamev I'm thinking that maybe this PR targets a feature branch unified-parameters and until we've converted all of the other LLMs we'd hold back rolling this out. What do you think?

@codenamev
Copy link
Contributor Author

@codenamev I'm thinking that maybe this PR targets a feature branch unified-parameters and until we've converted all of the other LLMs we'd hold back rolling this out. What do you think?

Is there a branch named that, or are you just saying we should do all or nothing? Heh, I meant to submit a Draft PR here just showcasing how it would work with the most common LLM (OpenAI). If we can agree on the abstraction and new parameter flow, I'd be happy to run through all the existing LLMs and update them to mimic the same behavior :D

@andreibondarev
Copy link
Collaborator

@codenamev I'm thinking that maybe this PR targets a feature branch unified-parameters and until we've converted all of the other LLMs we'd hold back rolling this out. What do you think?

Is there a branch named that, or are you just saying we should do all or nothing? Heh, I meant to submit a Draft PR here just showcasing how it would work with the most common LLM (OpenAI). If we can agree on the abstraction and new parameter flow, I'd be happy to run through all the existing LLMs and update them to mimic the same behavior :D

Yeah, I just meant that we should do all or nothing. I can cut a new origin/unified-parameters branch if you think it would be helpful.

…on the LLM::Base instance"

This reverts commit 3a20278796fce6f1649e6425e70822e49eb16856.
@andreibondarev andreibondarev self-requested a review May 10, 2024 13:59
@andreibondarev
Copy link
Collaborator

andreibondarev commented May 10, 2024

@codenamev I just noticed that the response from Ollama comes back serialized:

irb(main):005> llm.chat messages: [{role: "user", content: "Hey"}], stop: ["ello"]

=>
#<Langchain::LLM::OllamaResponse:0x00000001299d6118
 @model="llama3",
 @prompt_tokens=nil,
 @raw_response=
  "{\"model\":\"llama3\",\"created_at\":\"2024-05-10T16:54:49.041769Z\",\"message\":{\"role\":\"assistant\",\"content\":\"Hey\"},\"done\":false}\n{\"model\":\"llama3\",\"created_at\":\"2024-05-10T16:54:49.062254Z\",\"message\":{\"role\":\"assistant\",\"content\":\"!\"},\"done\":false}\n{\"model\":\"llama3\",\"created_at\":\"2024-05-10T16:54:49.082952Z\",\"message\":{\"role\":\"assistant\",\"content\":\" How\"},\"done\":false}\n{\"model\":\"llama3\",\"created_at\":\"2024-05-10T16:54:49.103971Z\",\"message\":{\"role\":\"assistant\",\"content\":\"'s\"},\"done\":false}\n{\"model\":\"ll
  ...

This is on main branch:

irb(main):003> llm.chat messages: [{role: "user", content: "Hey"}]
=>
#<Langchain::LLM::OllamaResponse:0x00000001255f63d0
 @model="llama3",
 @prompt_tokens=nil,
 @raw_response=
  {"model"=>"llama3",
   "created_at"=>"2024-05-10T16:58:46.122462Z",
   "message"=>{"role"=>"assistant", "content"=>"Hey! How's it going?"},

Would you happen to know why this was happening?

@andreibondarev
Copy link
Collaborator

@codenamev Tremendous effort, great job!

@andreibondarev andreibondarev merged commit b8169dd into patterns-ai-core:main May 10, 2024
5 checks passed
@codenamev codenamev deleted the vs-unified-parameters branch May 10, 2024 23:08
@drnic
Copy link
Contributor

drnic commented May 10, 2024

Huge effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants