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

Feature request/idea: Use generic functions to extract provider errors #38

Closed
r0man opened this issue Mar 24, 2024 · 10 comments
Closed

Comments

@r0man
Copy link
Contributor

r0man commented Mar 24, 2024

Hi @ahyatt,

since we are looking into error handling in the plz branch, I have a feature request/idea. Let me describe my problem first:

I have a custom LLM provider based on llm-openai-compatible. This provider talks to a server that is a proxy/gateway to the OpenAI API. With the switch to plz I could simplify it a lot (before all this curl code lived in my provider). I now basically set some curl arguments like the certificate, the private key and a header, and then just call the next method. Most of my code now look like this:

(cl-defmethod llm-chat-async ((provider nu-llm-openai) prompt response-callback error-callback)
  (nu-llm-request-with-curl-options (nu-llm-openai--curl-options provider)
    (cl-call-next-method provider prompt response-callback error-callback)))

This works well, except for one edge case. Unfortunately the server I'm dealing with is not consistent with its error messages. In rare cases like an authentication error, or a proxy error, the server returns errors in the HTTP response body that don't follow the error specification of the OpenAI API. Those errors are not from OpenAI, but from the proxy.

When this happens I get a backtrace with the current code, since the OpenAI provider assumes a specific format. I can't really fix this without copy and pasting the implementations of all those llm-xxx functions that make HTTP requests. Wrapping the response-callback or the error-callback in the example above won't work, since that wrapped code would run after the error occurred.

From what I have seen so far, most of the time we extract 2 things from some data:

  • An error code
  • An error message

So, I was wondering if we could maybe do something similar to what you did here: a35d2ce Something like this:

(cl-defgeneric llm-error-code (provider data)
  "Extract the error code from the DATA of PROVIDER.")

(cl-defgeneric llm-error-message (provider data)
  "Extract the error message from the DATA of PROVIDER.")

If we would have a mechanism like this, I could provide good error messages to my users without re-implementing a lot.

Wdyt?

@ahyatt
Copy link
Owner

ahyatt commented Mar 24, 2024

I think this seems reasonable, at least for Open AI. I'm not sure it's needed for other implementations, but maybe it makes sense to simplify the code. Let me think and look into it. We don't need an llm-error-code, though, anything like that should go into the error message itself.

I'll try to fool around with this idea today, or soon, and let you know how it goes. Thanks for the thoughtful suggestion!

@r0man
Copy link
Contributor Author

r0man commented Mar 24, 2024

Ok, perfect. Thanks!

@ahyatt
Copy link
Owner

ahyatt commented Mar 27, 2024

I've done this, but haven't pushed it yet. One thing that is stopping me is that I'd like to try an alternate idea, which is making it very easy to define a new provider, via macros. I think ultimately that will be a better solution, if it works out, which I'm in the middle of trying to verify.

@ahyatt
Copy link
Owner

ahyatt commented Mar 27, 2024

Here's an example I'm currently experimenting with. It works, but I haven't figured out how to get edebug to work on it yet.

(defllm-embedding-provider llm-openai (provider string response)
  :precheck (llm-openai--check-key provider)
  :url (llm-openai--url provider "embeddings")
  :headers `(("Authorization" . ,(format "Bearer %s" (llm-openai-key provider))))
  :request (llm-openai--embedding-request (llm-openai-embedding-model provider) string)
  :error-extractor (llm-openai--error-message provider response)
  :result-extractor (llm-openai--embedding-extract-response response))

@r0man
Copy link
Contributor Author

r0man commented Mar 28, 2024

Hi @ahyatt ,

thanks for working on this. I'm not convinced a macro will simplify making a new provider. It's a new little language people need to learn on top of looking into how a LLM provider works. I think I would prefer the ability to build my provider via specializing generic functions. That way I'm mostly in control, can leverage what generic functions provide (specialize certain functions on my own provider, call the "next" method, and customize methods via :before :around) and it works with edebug.

The code for my custom provider is actually very straightforward and can easily be understood. The extension points I needed were:

  • Add my own headers (solved with llm-openai--headers)
  • Use a certificate and private key (solved with binding plz-curl-args in the methods that do HTTP requests and calling the next method)
  • Transforming some errors from the proxy I use into an OpenAI compatible format (would be solved by a generic function that extracts errors and/or the response).

@ahyatt
Copy link
Owner

ahyatt commented Mar 28, 2024

Thanks for the response. Maybe we can have a solution that has aspects of both. I like the macro solution because it solves a very concrete problem for me: the structure for these methods are all exactly the same, and it's non-trivial, so it's difficult to make improvements to every provider. Perhaps using a method with lambdas and not a macro would be good. I can give it a shot, and either solution could still allow generic functions in Open AI. I think if we didn't use a macro but used a normal function, you could do the plz-curl-args binding.

Anyway maybe I'll just release the simple solution of having generic functions, and work on a macro or function that simplifies actually defining these, and consider these separate work.

@ahyatt
Copy link
Owner

ahyatt commented Mar 28, 2024

I think I have a better solution that should solve all concerns. I'll create a "standard provider" implementation, and all of today's implementations can have methods for getting the url, extracting errors, extracting the response, etc. This is nice, because it mostly means that we can just rename existing methods to do the same, make sure they are standardized, and remove the actual llm-chat etc. methods. And it can be debugged. Around methods you can do either way. I'll have to figure out whether I want to obsolete the open AI generic methods or just have some duplication. So I'm going to pursue this, and should be it out today or tomorrow to try out.

@r0man
Copy link
Contributor Author

r0man commented Mar 28, 2024

Sounds like a good plan

ahyatt added a commit that referenced this issue Mar 30, 2024
@ahyatt
Copy link
Owner

ahyatt commented Apr 7, 2024

In both main and plz branch, this now is how everything works. Thank you for the suggestion!

@ahyatt ahyatt closed this as completed Apr 7, 2024
@r0man
Copy link
Contributor Author

r0man commented Apr 7, 2024

Thanks for implementing this. It is working great!

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

No branches or pull requests

2 participants