-
-
Notifications
You must be signed in to change notification settings - Fork 225
Improve OpenRouter Support #144
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
base: main
Are you sure you want to change the base?
Conversation
696e779
to
186998a
Compare
@crmne What would you like to see to get this merged in? It improves OpenRouter support a lot, and I want to prevent someone else from duplicating work |
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.
Why does this PR changes anthropic, bedrock, deepseek, gemini, ollama, and openai too?
@crmne Because the current setup doesn't totally allow code reuse through extension, this tweaks it to allow that to work as needed while cleaning up some of the usage that was patched in. We can remove these tweaks. My expectations is that you'd like to continue to be able to share code across providers, and continue to use this module_function style. At the intersection of these, when Provider A makes use of Provider B module it moves into B's namespace; Which means when it needs to call back to Provider A's version of something, it's unavailable and will call Provider B's version. This is likely going to lead to some unexpected bugs. These changes copy Provider's B implantation into Provider A namespace, so that calls can be directed back to Provider A implantation when it has been defined. |
@sirwolfgang could you please move these improvements in another PR? I'd like to merge the improved OpenRouter support but don't want to change half of the library for it. |
@crmne I can move some of them, but not all of them. I made this changes because I was simply unable to extend the OpenAI code and have it work within the OpenRouter namespace. So I can remove the Do you still wish for me to remove the improvements from the other files? |
Yes. |
Updated |
content: Media.format_content(msg.content), | ||
content: self::Media.format_content(msg.content), |
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.
Was this necessary?
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.
Yes, as previously explained, the module-based system has scoping issues. This file is deeply called by the other modules, so in order to have this file call the correct version of the Media
file, which is not the OpenAI version, it needs to be scoped to the current runtime.
I attempted to make this fix universal, so that other people don't run into this bug; you asked me to roll it back where it wasn't necessary. So I did, but I can not roll it back in this location if I want the correct version of the Media object to be invoked at runtime.
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.
Thanks for the explanation, makes sense!
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.
Why you needed to implement all this? Check the Ollama provider, it only implements the messages that override what OpenAI already provides
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.
Why you needed to implement this as well? Check the Ollama provider, it only implements the messages that override what OpenAI already provides. And here there's nothing regarding the other provider options.
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.
OpenRouter uses a different format than OpenAI in order to support additional functionality. This is to support that.
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.
We have tests that work for OpenRouter media, including images, PDFs, etc.
spec/ruby_llm/chat_pdf_spec.rb
Outdated
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.
Why there is this test? Shouldn't there be simply a test with the options that you are implementing in this PR?
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.
My implementation in this PR affects the interface of usage. This test is to ensure that both this PR and future PRs do not break functionality. Generally, increasing test coverage is considered desirable.
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.
What exactly is affected in the interface? Again, there are already comprehensive tests for PDFs and other file formats already.
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.
remove
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.
remove
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.
remove
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.
remove
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.
remove
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.
remove
This fixes issues with OpenRouter support by splitting the media handling off of OpenAI's implementation, which is a little different. As part of that it also improves the way Media is loaded by Chat to ensure each provider is running it's own version, allowing for each extension by others.
Currently this works with either, making this default a URI option.
This interface does limit us from being able to pass in contents itself. This interface should likely be expanded or made more explicit to support raw files or already base64 encoded files.
Additionally this PR exposes the ability to set OpenRouter's Header settings and Provider Settings: