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

Customization of gptel-backend causes recursive require error, prevents gptel from loading #556

Open
jeffvalk opened this issue Jan 8, 2025 · 8 comments
Labels
bug Something isn't working

Comments

@jeffvalk
Copy link

jeffvalk commented Jan 8, 2025

First of all, thanks for gptel!

Here's what I'm seeing...

Bug Description

When customizing the value of gptel-backend, which is a defcustom, all backends fail to load with a recursive require error except for those provided in the gptel-openai feature (ChatGPT, Azure, and GPT4All).

This was previously reported as #239, and still exists.

Backend

Ollama, Gemini, Kagi, Anthropic, PrivateGPT

Steps to Reproduce

With emacs -Q:

(add-to-list 'load-path "~/.config/emacs/elpa/compat-30.0.2.0")
(add-to-list 'load-path "~/.config/emacs/elpa/transient-20250103.1731")
(add-to-list 'load-path "~/.config/emacs/elpa/gptel-20250107.34")

(use-package gptel
  :commands (gptel gptel-make-ollama) ; autoloads
  :custom ((gptel-backend (gptel-make-ollama "Ollama"))))
                     ; or (gptel-make-gemini "gemini")
                     ; or (gptel-make-kagi "kagi")
                     ; or (gptel-make-anthropic "anthropic")
                     ; or (gptel-make-privategpt "privategpt")

(gptel) ; => recursive load error

Additional Context

Cause

Because the value expected by gptel-backend is an object instance, user customization must invoke a constructor function to set this value. The constructor functions are autoloaded. The error occurs because gptel-ollama, gptel-gemini, gptel-kagi, gptel-anthropic, and gptel-privategpt all(require 'gptel). This creates a cyclic dependency: loading gptel autoloads the backend, which tries to load gptel.

In contrast, gptel-openai does not require gptel. This backend does not have this issue.

Possible fix

The gptel-openai backend uses defvar and declare-function at the top of the file for referenced symbols instead of (require 'gptel). It seems that the other backend files might do this as well.

Workaround

As noted in #239, using setq avoids triggering the issue:

(use-package gptel
  :defer t
  :config (setq gptel-backend (gptel-make-ollama "ollama"))) ; workaround, since
; :custom (gptel-backend (gptel-make-ollama "ollama"))         <- this causes infinite recursion

However, gptel-backend is a defcustom, and not being able to set it using Emacs' customization mechanism is confusing (and likely a bug).

Backtrace

Debugger entered--Lisp error: (error "Recursive load" "...")
  (gptel-make-ollama "Ollama")
  eval((gptel-make-ollama "Ollama"))
  custom-initialize-reset(gptel-backend (funcall #'#f(compiled-function () #<bytecode 0x1980fa740ac14>)))
  custom-declare-variable(gptel-backend (funcall #'#f(compiled-function () #<bytecode 0x1980fa740ac14>)) "LLM backend to use.\n\nThis is the default \"backend\"..." :safe always :type (choice (const :tag "ChatGPT" #s(gptel-openai :name "ChatGPT" :host "api.openai.com" :header #f(compiled-function () #<bytecode 0xf1304cc01551e1c>) :protocol "https" :stream t :endpoint "/v1/chat/completions" :key gptel-api-key :models (gpt-4o gpt-4o-mini gpt-4-turbo gpt-4 gpt-4-turbo-preview gpt-4-0125-preview o1-preview o1-mini gpt-4-32k gpt-4-1106-preview gpt-3.5-turbo gpt-3.5-turbo-16k) :url "https://api.openai.com/v1/chat/completions" :request-params nil :curl-args nil)) (restricted-sexp :match-alternatives (gptel-backend-p 'nil) :tag "Other backend")))
  byte-code("\301\302\303\304\305DD\306\307\310\311\312\313\314\315\10F\316BB&\7\207" [gptel--openai custom-declare-variable gptel-backend funcall function #f(compiled-function () #<bytecode 0x1980fa740ac14>) "LLM backend to use.\n\nThis is the default \"backend\"..." :safe always :type choice const :tag "ChatGPT" ((restricted-sexp :match-alternatives (gptel-backend-p 'nil) :tag "Other backend"))] 12)
  (gptel-make-ollama "Ollama")
  eval((gptel-make-ollama "Ollama"))
  custom-initialize-reset(gptel-backend (funcall #'#f(compiled-function () #<bytecode 0x1980fa740ac14>)))
  custom-declare-variable(gptel-backend (funcall #'#f(compiled-function () #<bytecode 0x1980fa740ac14>)) "LLM backend to use.\n\nThis is the default \"backend\"..." :safe always :type (choice (const :tag "ChatGPT" #s(gptel-openai :name "ChatGPT" :host "api.openai.com" :header #f(compiled-function () #<bytecode 0xf1304cc01551e1c>) :protocol "https" :stream t :endpoint "/v1/chat/completions" :key gptel-api-key :models (gpt-4o gpt-4o-mini gpt-4-turbo gpt-4 gpt-4-turbo-preview gpt-4-0125-preview o1-preview o1-mini gpt-4-32k gpt-4-1106-preview gpt-3.5-turbo gpt-3.5-turbo-16k) :url "https://api.openai.com/v1/chat/completions" :request-params nil :curl-args nil)) (restricted-sexp :match-alternatives (gptel-backend-p 'nil) :tag "Other backend")))
  byte-code("\301\302\303\304\305DD\306\307\310\311\312\313\314\315\10F\316BB&\7\207" [gptel--openai custom-declare-variable gptel-backend funcall function #f(compiled-function () #<bytecode 0x1980fa740ac14>) "LLM backend to use.\n\nThis is the default \"backend\"..." :safe always :type choice const :tag "ChatGPT" ((restricted-sexp :match-alternatives (gptel-backend-p 'nil) :tag "Other backend"))] 12)
  (gptel-make-ollama "Ollama")
  eval((gptel-make-ollama "Ollama"))
  custom-initialize-reset(gptel-backend (funcall #'#f(compiled-function () #<bytecode 0x1980fa740ac14>)))
  custom-declare-variable(gptel-backend (funcall #'#f(compiled-function () #<bytecode 0x1980fa740ac14>)) "LLM backend to use.\n\nThis is the default \"backend\"..." :safe always :type (choice (const :tag "ChatGPT" #s(gptel-openai :name "ChatGPT" :host "api.openai.com" :header #f(compiled-function () #<bytecode 0xf1304cc01551e1c>) :protocol "https" :stream t :endpoint "/v1/chat/completions" :key gptel-api-key :models (gpt-4o gpt-4o-mini gpt-4-turbo gpt-4 gpt-4-turbo-preview gpt-4-0125-preview o1-preview o1-mini gpt-4-32k gpt-4-1106-preview gpt-3.5-turbo gpt-3.5-turbo-16k) :url "https://api.openai.com/v1/chat/completions" :request-params nil :curl-args nil)) (restricted-sexp :match-alternatives (gptel-backend-p 'nil) :tag "Other backend")))
  byte-code("\301\302\303\304\305DD\306\307\310\311\312\313\314\315\10F\316BB&\7\207" [gptel--openai custom-declare-variable gptel-backend funcall function #f(compiled-function () #<bytecode 0x1980fa740ac14>) "LLM backend to use.\n\nThis is the default \"backend\"..." :safe always :type choice const :tag "ChatGPT" ((restricted-sexp :match-alternatives (gptel-backend-p 'nil) :tag "Other backend"))] 12)
  (gptel-make-ollama "Ollama")
  eval((gptel-make-ollama "Ollama"))
  custom-initialize-reset(gptel-backend (funcall #'#f(compiled-function () #<bytecode 0x1980fa740ac14>)))
  custom-declare-variable(gptel-backend (funcall #'#f(compiled-function () #<bytecode 0x1980fa740ac14>)) "LLM backend to use.\n\nThis is the default \"backend\"..." :safe always :type (choice (const :tag "ChatGPT" #s(gptel-openai :name "ChatGPT" :host "api.openai.com" :header #f(compiled-function () #<bytecode 0xf1304cc01551e1c>) :protocol "https" :stream t :endpoint "/v1/chat/completions" :key gptel-api-key :models (gpt-4o gpt-4o-mini gpt-4-turbo gpt-4 gpt-4-turbo-preview gpt-4-0125-preview o1-preview o1-mini gpt-4-32k gpt-4-1106-preview gpt-3.5-turbo gpt-3.5-turbo-16k) :url "https://api.openai.com/v1/chat/completions" :request-params nil :curl-args nil)) (restricted-sexp :match-alternatives (gptel-backend-p 'nil) :tag "Other backend")))
  byte-code("\301\302\303\304\305DD\306\307\310\311\312\313\314\315\10F\316BB&\7\207" [gptel--openai custom-declare-variable gptel-backend funcall function #f(compiled-function () #<bytecode 0x1980fa740ac14>) "LLM backend to use.\n\nThis is the default \"backend\"..." :safe always :type choice const :tag "ChatGPT" ((restricted-sexp :match-alternatives (gptel-backend-p 'nil) :tag "Other backend"))] 12)
  (gptel)
  (progn (gptel))
  eval((progn (gptel)) t)
  elisp--eval-last-sexp(nil)
  eval-last-sexp(nil)
  funcall-interactively(eval-last-sexp nil)
  call-interactively(eval-last-sexp nil nil)
  command-execute(eval-last-sexp)

Log Information

No response

@jeffvalk jeffvalk added the bug Something isn't working label Jan 8, 2025
@karthink
Copy link
Owner

karthink commented Jan 8, 2025

This creates a cyclic dependency: loading gptel autoloads the backend, which tries to load gptel.

That can't quite be the problem, because by this logic the recursive load would happen if you used setq before loading gptel as well:

(use-package gptel
  :init (setq gptel-backend (gptel-make-ollama "ollama"))
  :defer t)

But this runs without errors for me. In fact autoloading wouldn't work at all if this was a problem. For example, gptel.el (file A) also calls autoloaded functions like gptel-menu, which is in gptel-transient.el (file B), where the latter has (require 'gptel) in it. (A calls function from B, where B requires A)

I'll have to look carefully at what custom-theme-set-variables does differently. The custom machinery is... complicated.

@jeffvalk
Copy link
Author

jeffvalk commented Jan 8, 2025

The custom machinery does seem a primary factor; and it is complicated. Instead of diving too deeply into that, may I ask... Is there a reason the gptel-openai backend has one dependency design and all the other backends have a different one?

@jeffvalk
Copy link
Author

Replacing

(require 'gptel)

with

(defvar gptel-backend (gptel-make-openai "OpenAI"))

in gptel-ollama.el allows customization of gptel-backend to succeed with that backend.

It seems that line is necessary because the non-OpenAI backends specify :include gptel-backend, which is expected to be the OpenAI backend. This makes any backend set in a user's init file derive from gptel-openai.

If there are features to include in all backends, might a common base of some sort make sense?

@karthink
Copy link
Owner

Is there a reason the gptel-openai backend has one dependency design and all the other backends have a different one?

See #227

allows customization of gptel-backend to succeed with that backend.

This works because (require 'gptel) was removed, not because you added (defvar gptel-backend ...).

It seems that line is necessary because the non-OpenAI backends specify :include gptel-backend, which is expected to be the OpenAI backend. This makes any backend set in a user's init file derive from gptel-openai.

If there are features to include in all backends, might a common base of some sort make sense?

You are mixing up gptel-backend the datatype with gptel-backend the variable.

gptel-backend is a generic struct datatype that all backend types derive from, including the gptel-openai type.

gptel-backend is a user option/variable that can be set to any instance of gptel-backend (the type).

@jeffvalk
Copy link
Author

Ah! Thanks for that clarity!

If we autoload the gptel-backend struct:

;;;###autoload
(cl-defstruct (gptel-backend ...))

then we can remove (require 'gptel) from the backends, which prevents the recursive require error.

Without autoloading this, the other backends do require gptel-openai, which they get transitively by requiring gptel for the reasons noted in #227.

I might also consider moving the base struct for all backends out of a specific backend implementation for stylistic reasons. But as long as it is autoloaded, the recursive require error is avoided.

@karthink
Copy link
Owner

If we autoload the gptel-backend struct

A call to the autoloaded gptel-make-ollama (say) will make a call to the autoloaded gptel-backend which will load gptel-openai -- depending on a chain of autoloads like this is not very robust either. It's also hard to debug or understand when something goes wrong.

I have an idea for a solution via moving code around that doesn't require so much autoloading, but I'll work on it after merging the tool-use branch. (Essentially I plan to move all the generic backend definition code and the network code to a single file that is not gptel.el, and keep only the UI code in gptel.el.)

I might also consider moving the base struct for all backends out of a specific backend implementation for stylistic reasons.

The only way to do this right now is via chained autoloading, which I don't want to do as noted in #227.


Taking a step back, it would be good if custom-set-* only evaluated its expressions once, when the package is first loaded. I think I need to understand why its current behavior evaluates code every time the feature (gptel, in this case) is loaded. This seems unnecessary at best and a bad idea generally.

@jeffvalk
Copy link
Author

depending on a chain of autoloads like this is not very robust

Agreed completely

It's also hard to debug or understand when something goes wrong

I will personally attest 🙂

I have an idea for a solution via moving code around that doesn't require so much autoloading, but I'll work on it after merging the tool-use branch. (Essentially I plan to move all the generic backend definition code and the network code to a single file that is not gptel.el, and keep only the UI code in gptel.el.)

Excellent! This is similar to what I had in mind, but considerably more comprehensive.

@astoff
Copy link

astoff commented Jan 13, 2025

Since gptel-make-... asks for a NAME, why not allow gptel-backend to simply refer to that name? So for instance (gptel-make-ollama "Ollama") and (setq gptel-backend 'Ollama) instead of (setq gptel-backend (potentially complicated form)).

I think this would separate the backend definition step from the backend selection step and avoid this type of confusion. (There's no documentation about it I believe, but defining two backends with the same name probably leads to trouble, so it's effectively a new namespace.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants