ahyatt / llm

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

Add Plz #26

Closed r0man closed 6 months ago

r0man commented 6 months ago

Hi @ahyatt

This adds plz to the LLM library. Or at least temporarily.

plz.el is a slightly modified version of upstream.

  1. It adds a slot for the process object to the response struct, so we can access properties of the process in various places. In our discussion about the plz streaming PR, alphapapa suggested this at some point.

  2. The plz function has an additional option called :process-filter that allows setting the process filter when the curl process is created with make-process. This is needed to set the process filter in the synchronous and asynchronous cases. In the asynchronous case you could to this immediately after the process is created. In the synchronous case you can't get at the process object to install a filter.

Ideally we could upstream the above.

plz-media-type.el contains code that can be used implement and customize response decoding for various media types. It ships with default media types for application/json, application/html, application/xml and the default application/octet-stream. A media type can support "normal" and "straming" formats.

plz-event-source.el contains a media type implementation for text/event-stream, aka server sent events, and an implementation of an event source class and parser according to the HTML living standard.

llm-request-plz.el A copy with the same functionality as the llm-request module, but using plz instead of url-retrieve. We could eventually rename the llm-request-plz module to llm-request once we are done with adding support for curl to all providers.

Some open questions and suggestions:

Wdyt?

alphapapa commented 6 months ago

Copying my comments from the commit:

@r0man A few thoughts:

  1. It's generally not a good idea to just copy another library into another project. The behavior will then depend on the load order, which will decide which copy gets loaded.
  2. If you must do this, you should rename the symbols and file, probably prefixing plz with something else.
  3. It would be best to limit the modifications to plz to those strictly necessary. As much as possible, the functionality should be implemented on top of plz's existing functionality.
r0man commented 6 months ago

Hi @alphapapa,

this is a PR to the plz branch to get the ball rolling, not the main branch. The idea is to use this branch until all providers have been changed to use curl, possibly make changes to the plz-xxx files, until we know better howto use plz from the llm library. Once this is ready to be merged into the main branch we get rid of the files and/or rename them.

alphapapa commented 6 months ago

Understood. Thanks.

ahyatt commented 6 months ago

Thanks for doing this. I agree about the concern, but the purpose here is to vet ideas, not have this code ever see the light of day. I'll merge this in.

About the continuous integration, we already have one set up, so no problems there.

If you want to help convert, that's great, but I think it's good that I convert a bunch of them, which I can start today, so I can provide feedback on your API and let you know about problems with a client perspective.

Don't worry about plz testing - the only tests I think it makes sense to run are the llm ones. You should test Open AI with (llm-tester-all your-openai-provider) and inspect the buffer to make sure there are no instances of "ERROR".

r0man commented 6 months ago

@ahyatt Nice, didn't know about llm-tester-all!

Ok, sounds like a plan. I'll wait for your feedback.

Maybe we can use SSE for Vertex as well. I recently found this one here: https://cloud.google.com/vertex-ai/generative-ai/docs/learn/streaming#rest-sse

If you have a curl response dump for the other "streaming" format(s), I could look into adding support for those formats.

ahyatt commented 6 months ago

The SSE thing is interesting, they must have added that recently. But it doesn't support Gemini, so I'd prefer to wait until it does.

One question: do you want me to ask questions and have discussion here or should I just make assumptions and check in fixes? For example, I just got llm-vertex embeddings to work, but had to change llm-request-plz-async to return (plz-response-body response) instead of response in the success callback. I can check in my change, but perhaps we should discuss these issues first. Let me know what makes the most sense to you.

r0man commented 6 months ago

Yes, I also had the impression SSE on Vertex does not work in all situations. I'm fine with waiting.

Please feel free to check in fixes. I'm also happy to answer questions. As you prefer. I comment on the other PR.