karthink / gptel

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

Saving and restoring context files/buffers #439

Closed benthamite closed 3 weeks ago

benthamite commented 3 weeks ago

I think it would be useful to have the option of storing the files or buffers that are part of a gptel session’s context for later use. Such lists of files or buffers could be stored as an additional org property, in the same way as the model, backend, and prompt are stored under GPTEL_MODEL, GPTEL_BACKEND and GPTEL_SYSTEM, respectively. It seems this could be done easily via gptel-org-set-properties, obtaining the relevant list from gptel-context--alist. Would you be interested in a PR to provide this functionality?

karthink commented 3 weeks ago

On first thought, I'm not sure this is a good idea for three reasons, one conceptual, and two related to practical difficulties with restoring/saving the state.

  1. Conceptually, I think of the context as part of the text that's sent, and not a model parameter, and the metadata is meant to store model parameters. That said, the system message is technically not a model parameter either, so this isn't a strong distinction.

  2. How do you handle conflicts in the values of gptel--context-alist after restoring the state? You'd have to let-bind gptel--context-alist to the stored values when running gptel-send. In this case, you'd be ignoring the current top-level value of gptel--context-alist. What happens if the user wants to include a file? Either the saved value of gptel--context-alist takes precedence, or the current one does, or we have do some kind of merge action. Seems like a mess however you slice it. Currently the saved values take precedence for the other parameters, which isn't much of a problem for the system message etc.

  3. When saving the state, what happens to the included buffer text regions (i.e. not files)? These are valid for the session only. If we don't store them, we are not capturing gptel--context-alist correctly. If we do, they're going to be invalid in the future anyway. Finally, it's not clear how to actually store them since they're overlays -- overlays don't have a self-contained printable text representation.

If we can find a way to implement this feature that is (i) unambiguous, (ii) complete (not partial save/restore), (iii) reliable and (iv) consistent with how the other model parameters are saved/restored, I think we can add it.

benthamite commented 3 weeks ago

Mmh, I think those are all very reasonable worries.

I suggested this enhancement because I sometimes find myself in a situation where the context consists of several hand-picked files (typically because including all the files in a project would be too costly, or would slow things down too much). In such cases, I may want to temporarily remove this context to make an unrelated request, or may have to restart Emacs for some reason. Being able to restore those context files quickly after the interruption would be a nice convenience.

A way to address your concerns might be to save and restore the context state via an explicit user action. Then any existing context would be wiped out when the user agrees to restore a saved context, and once a context is restored any further modifications are just ordinary file/buffer context additions or removals. But I’m not very familiar with the internals of gptel so I may be overlooking some relevant considerations.

karthink commented 3 weeks ago

This might be best done via a helper function outside of gptel for now. It might be a good idea to add a hook to gptel, something like gptel-save-state-hook. This runs when (after) gptel's model parameters are written to the file. Then you can run your bespoke function to save the context files in this hook. What do you think?

benthamite commented 3 weeks ago

Yes, this sounds good!

karthink commented 3 weeks ago

I added gptel-save-state-hook. It runs before the state is written to disk by gptel--save-state. This is because you might want to do modify the buffer in some way in this hook that is reflected in the state parameters written to disk. If we flip this order, properties like GPTEL_BOUNDS might be wrong as a result.

To restore the state you save via gptel-save-state-hook, you can just use gptel-mode-hook. Be sure to check against the buffer major-modewhen saving and restoring additional data.

benthamite commented 3 weeks ago

Thank you!