Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

gptel--system-message is not robustly initialised. #416

Closed
metachip opened this issue Oct 12, 2024 · 4 comments
Closed

gptel--system-message is not robustly initialised. #416

metachip opened this issue Oct 12, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@metachip
Copy link

After reviewing the README.org file, I don't see any explicit warning about renaming or removing the 'default' prompt in the gptel-directives alist. I think this is potential issue because:

  1. The package relies on the presence of a 'default' key in gptel-directives to initialize gptel--system-message.
  2. The customization interface allows one to modify this variable name without warning about the consequences.
  3. Renaming, removing or setting as nil the 'default' prompt uncovers a latent bug.

My suggestion:

  1. Add a clear warning in the README.org file about not removing or renaming the 'default' prompt in gptel-directives and/or
  2. Modify the definition of this variable to be more resilient to changes in gptel-directives. For example:
    (defvar gptel--system-message
      (or (alist-get 'default gptel-directives)
        "You are a helpful assistant living in Emacs. Respond concisely.")
      "The system message used by gptel.")

Alternatively or as well, it might be a good idea to modify the defcustom for gptel-directives to use a custom widget that would disallow the user from doing this in the first place. Perhaps something like:

(defcustom gptel-directives
  '((default . "You are a helpful assistant living in Emacs. Respond concisely.")
    ;; ... other directives ...
    )
  "System prompts (directives) for the LLM."
  :type '(list (cons :format "%v"
                     (const :format "" default)
                     (string :tag "Default prompt"))
               (repeat (cons :format "%v"
                             (symbol :tag "Name")
                             (string :tag "Prompt"))))
  :group 'gptel)

This would ensure the first item is always named default and that it can't be renamed, it can't be deleted. It allows customisation of the prompts value and it permits adding, modifying and removing other named prompts.

I think there would also need to :set to ensure the default value is never nil.

:set (lambda (sym val)
       (set-default sym
                    (cons (cons 'default (or (cdar val)
                                             "You are a helpful assistant living in Emacs. Respond concisely."))
                          (cdr val))))

I've also added :group 'gptel as the group. Shouldn't it have been there in the first place?

@karthink karthink added the enhancement New feature or request label Oct 20, 2024
@karthink
Copy link
Owner

Thanks for bringing this up -- the system message system needs some work on the whole, I'll work on this when I address the other issues. (Specifically, some users are finding the way the system message is stored/restored/applied confusing, so I have to rework the presentation. I also plan to make it possible to specify functions instead of strings so the system message can be dynamically generated by the user.)

I've also added :group 'gptel as the group. Shouldn't it have been there in the first place?

This is not required, the customization system adds this automatically to provided features unless the library name is nonstandard.

@metachip
Copy link
Author

A suggestion when you do get to this, I find the system message one prefers often depends on the model that is selected. I find when having a dialogue, in which I switch models mid-chat, I also sometimes need to switch the system prompt to better match the new model.

One suggestion might be to tie the selected system message to the selected model. If a model that one has not yet used in a given chat is then selected, it would inherit the system message prevailing from the previous model. If one then changes it, the new system message only applies to the newly selected model so that returning to the previous one restores the system message that prevailed at that time.

@karthink
Copy link
Owner

karthink commented Jan 2, 2025

A suggestion when you do get to this, I find the system message one prefers often depends on the model that is selected. I find when having a dialogue, in which I switch models mid-chat, I also sometimes need to switch the system prompt to better match the new model.

This appears to be a need that many users have. There are many discussions threads where people have asked how to implement a "presets" feature. I think it might make sense to add a presets feature to the package which is a bundle of configuration you can switch to all at once.

(gptel-make-preset
 "coding-preset"
 :system "system message for coding here..."    ; can be a function, see gptel-directives
 :backend gptel--anthropic-backend              ; or name of backend, like "Claude"
 :model   'claude-3-sonnet-20240229
 :context 'gptel-context-lsp
 :tools  (list gptel-tool-1 gptel-tool-2 ...)   ; list of gptel tools to supply
 :callback nil)

You can then select a preset from the transient menu, which sets all of these options at once. Via the "scope" switch in the menu, this preset can be set globally, in one buffer or just for the next request.

Some of these options/sources (like gptel-context-lsp) don't exist yet. :callback is basically a custom action you can specify instead of inserting the response, with nil being the default callback gptel uses.

In other LLM clients this is called an "agent", but really they're just a bundle of prompts+configuration.

@karthink
Copy link
Owner

karthink commented Jan 2, 2025

Modify the definition of this variable to be more resilient to changes in gptel-directives

I think this is the simplest solution. It's not any more robust than the others, but it's the easiest to understand.

karthink added a commit that referenced this issue Jan 2, 2025
* gptel.el (gptel--system-message): Add a default value for when
there is no 'default' key in `gptel-directives'.  Address #416.
@karthink karthink closed this as completed Jan 2, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants