karthink / gptel

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

feature/curl proxy support #69

Closed PalaceChan closed 1 year ago

PalaceChan commented 1 year ago

This is an amazing package.

In many organizations the openai api can only be accessed via proxy. This is something easily supported by curl. This PR proposes an (optionally empty) path to a proxy to enable gptel in these cases.

Implemented in two commits purely for expository reasons (easily squashed into one)

The first commit minimally shows how simple the change is, the crux of that diff being:

@@ -54,6 +61,12 @@ PROMPTS is the data to send, TOKEN is a unique identifier."
     ;; (push (format "--keepalive-time %s" 240) args)
     (push (format "-m%s" 60) args)
     (push "-D-" args)
+    (when (not (string-empty-p gptel-proxy-path))
+      (push "--proxy" args)
+      (push (format "%s" gptel-proxy-path) args)
+      (push "--proxy-negotiate" args)
+      (push "--proxy-user" args)
+      (push ":" args))

The second commit is a proposed cleanup of the overall function (from a bunch of single pushes) to leveraging list and append and also avoiding a reverse at the end. Both commits worked with and without a proxy server.

(GPT-4, together with gptel, actually helped with parts of the second commit)

karthink commented 1 year ago

Looks good, thank you.

The second commit is a proposed cleanup of the overall function (from a bunch of single pushes) to leveraging list and append and also avoiding a reverse at the end.

push + nreverse is how cl-loop generates lists, so this is not the case, BTW. The old and new versions expand to almost the same code:

Old, pcase-dolist version:

(pcase-dolist (`(,key . ,val) headers)
      (push (format "-H%s: %s" key val) args))

macroexpands to:

(cl-block nil
  (let
      ((--dolist-tail-- headers)
       x1262)
    (while --dolist-tail--
      (setq x1262
            (car --dolist-tail--))
      (progn
        (ignore
         (consp x1262))
        (let*
            ((x1263
              (car-safe x1262))
             (x1264
              (cdr-safe x1262)))
          (let
              ((key x1263)
               (val x1264))
            (push
             (format "-H%s: %s" key val)
             args))))
      (setq --dolist-tail--
            (cdr --dolist-tail--)))))
(nreverse (cons url args))

New, cl-loop version:

(cl-loop for (key . val) in headers
              collect (format "-H%s: %s" key val))

macroexpands to

(cl-block nil
  (let*
      ((--cl-var-- headers)
       (val nil)
       (key nil)
       (--cl-var-- nil))
    (while
        (consp --cl-var--)
      (setq val
            (car --cl-var--)
            key
            (pop val))
      (push
       (format "-H%s: %s" key val)
       --cl-var--)
      (setq --cl-var--
            (cdr --cl-var--)))
    (nreverse --cl-var--)))

Also a bunch of single pushes is no different from using list + append (performance-wise). I structured it like that originally because I wasn't sure if those Curl arguments would need to be able to be turned off individually depending on the needs of the package. As it turned out, there was no such need.

That said, the cleaned up version looks good for now, thanks.

PalaceChan commented 1 year ago

TIL on the cl-loop macro expand vs the pcase-dolist version

karthink commented 1 year ago

Okay, we are good to go, merging.

TIL on the cl-loop macro expand vs the pcase-dolist version

(push + nreverse is the most efficient way to generate a list in elisp, so every accumulator will use these two)

karthink commented 1 year ago

Cheers @PalaceChan