karthink / gptel

A simple LLM client for Emacs
GNU General Public License v3.0
1.21k stars 122 forks source link

Usage of aio #11

Closed minad closed 1 year ago

minad commented 1 year ago

I must admit, I am a bit ideological about avoiding dependencies in Emacs packages, which may break the package in the future. Does aio add much value or could we remove it as well, just using callbacks? I noticed that skeeto (the author of aio) is not actively using Emacs anymore, except Elfeed, which is not an ideal maintenance situation. I am happy to help out here with PRs of course, don't feel pressured to act on any of the issues I've opened.

karthink commented 1 year ago

I am not tied to the idea of using aio, since

I too prefer to avoid dependencies, thus no use of s, dash, deferred or the very common requests in this package. I wrote the curl interface from scratch as well, which requests provides. (I picked aio assuming there might be callback chains for continuous conversations, but the API turned out to be completely stateless.)

I wouldn't mind changing this code to use simple callbacks. But I'd like to get the package to a usable state first. The url-retrieval code is almost completely orthogonal to the UI and data processing concerns, so it can be done at any time. If you're willing to giving it a go that would be amazing!

karthink commented 1 year ago

Oh, switching away from aio should happen before writing the process filter for continuous updates (#9).

karthink commented 1 year ago

@minad I removed the aio dependency. This also makes the package much easier to debug/test. As of now gptel has no dependencies if you're using Emacs 28.1 or higher.

(If you would like to take a crack at writing a process filter for the "stream" API option, it should be much easier now!)

minad commented 1 year ago

Thank you, this is great!

minad commented 1 year ago

~But now it doesn't seem to work anymore (at least with curl). I always get Response error: finished^J. My recommendation would be to remove either one of the backends the curl or the url-based one. This will simplify development and there is practically no advantage in providing multiple backends. It is okay if your package requires curl.~

(EDIT: My mistake, since I am currently hacking on #13)

karthink commented 1 year ago

@minad I actually pushed a "fix" of sorts to this before I saw your edit. In your opinion is it more robust to match against (process-status process) or the status string passed to process sentinels? Specifically, I can check if the former is 'exit or the latter is the string finished\n. In this case I don't think it matters, but I switched to the former anyway. (I've never seen finished^J (what you got) as the status string in a callback before -- this is what is displayed in the header line.)

karthink commented 1 year ago

This will simplify development and there is practically no advantage in providing multiple backends.

I would actually like to avoid Curl and just use url-retrieve, but I haven't been able to figure out #4. I think it's reasonable to expect Curl to be available in an Emacs environment, so I'm okay with focusing on/developing the Curl backend while retaining url-retrieve as a basic fallback (i.e. no streaming responses etc).

minad commented 1 year ago

Why not just remove the url-retrieve backend? It doesn't add much value. For example Elfeed requires curl. My Osm package also relies on curl since I want to download many tiles asynchronously in parallel without slowing down Emacs. Generally I completely agree that it is nice to avoid dependencies. But this doesn't mean that we can make exceptions. Curl is very widely available and an established standard used by many tools and Emacs packages.