ahyatt / llm

A package abstracting llm capabilities for emacs.
GNU General Public License v3.0
142 stars 19 forks source link

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

Closed r0man closed 3 months ago

r0man commented 3 months ago

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:

So, I was wondering if we could maybe do something similar to what you did here: https://github.com/ahyatt/llm/commit/a35d2ceef13a57c15277fd6833e784b56b54f48d 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 commented 3 months ago

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 commented 3 months ago

Ok, perfect. Thanks!

ahyatt commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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:

ahyatt commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

Sounds like a good plan

ahyatt commented 3 months ago

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

r0man commented 3 months ago

Thanks for implementing this. It is working great!