Closed daedsidog closed 1 week ago
karthink
commented
3 months ago Thanks, I'll need some time to familiarize myself with this.
I synced up with the master branch of GPTel, and just added that in, but my transient menu seems to be vastly different from the previews I've seen.
Can you post a screenshot of your gptel transient menu? I'm not sure why you would see something different if you're on the master branch.
daedsidog
commented
3 months ago Can you post a screenshot of your gptel transient menu? I'm not sure why you would see something different if you're on the master branch.
Perhaps I am too out-of-the-loop an missed out the blatant instructions on how to get the transient menu to look like in the previews?
karthink
commented
3 months ago No worries, it's (setq gptel-expert-commands t)
This is a temporary measure while I figure out how much of the menu to expose by default without overwhelming new users.
On Fri, Mar 15, 2024, 6:23 PM daedsidog @.***> wrote:
Can you post a screenshot of your gptel transient menu? I'm not sure why you would see something different if you're on the master branch.
image.png (view on web) https://github.com/karthink/gptel/assets/41439659/d37669cf-374c-4fa2-83bc-30df20debd0c
Perhaps I was too out-of-the-loop an missed out the blatant instructions on how to get the transient menu to look like in the previews?
— Reply to this email directly, view it on GitHub https://github.com/karthink/gptel/pull/256#issuecomment-2000965169, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACBVOLAL5YGTJOHULYI6LW3YYONIFAVCNFSM6AAAAABEY27SVCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBQHE3DKMJWHE . You are receiving this because you commented.Message ID: @.***>
daedsidog
commented
3 months ago @karthink Under which circumstances does the GPTEL: overlay appears? I don't seem to have it like in your previews here:
karthink
commented
3 months ago It appears at the top of the user prompt the cursor is in -- so it might be out of view if your prompt is taller than the screen. If the region is active, that's the prompt boundary so it appears above the region.
I couldn't find a better place to put it. Putting it after the cursor didn't make much sense as a directive.
daedsidog
commented
3 months ago Odd, I don't seem to have it appear in any way:
Regardless, I think those overlays do not do what I initially thought they did. I assumed the overlays were directives the user sprinkles throughout the code, and then GPTel considers them.
For example, right now, what I've done is have the user just write comments prefixed with INSTRUCTION:, like so:
Whenever I refactored, those messages were considered by the model (well, not really, I just specified in my prompt to treat them specially, but there was code that actually checks that the instruction comment exists, otherwise refactoring was disabled).
Perhaps it is useful to what my configuration does to the above instruction comments. When I select the above comments and refactor, all it does is delete them and then insert the following:
I need you to refactor and/or write code. Write carefully with great respect to existing syntax, styling, & conventions, this also includes column width.
Do as instructed by the INSTRUCTION comments in the following source:
In buffer '*scratch*':
```
... (Line 6)
;; INSTRUCTION: write a function called 'f'
;; INSTRUCTION: make the above function do A B C
```
When writing code, do not delete existing comments. Do not fix what you may think are bugs, unless explicitly instructed to. Do not refactor things you were not instructed to. You are to remove instruction comments after they are completed.
You have more creative freedom when you are generating code, but you must still follow your instructions closely.
As opposed to the context, the source code you have been given has been erased from its original location. Therefore, you are expected to return it, fully. Of course, you are not to return the ellipses or line numbers. Remember, you may have been given partial code, so be mindful and return it in the same way you found it. Return **ALL** of the modified target code, leave nothing behind.
Return only code, nothing else. When documenting your work, make no reference to the instructions, so that an outside reader will think the code was written in a natural fashion.Then, it sends the above to the GPTel refactor as a user message.
I'm wondering what's the best way to have it in GPTel, so that users may customize this better. I think having directives in the code is very important, since they're easy to add/remove.
In my opinion, instead of using comments, we could embed those overlays in the buffer, and when the region is selected, those overlays will be baked into the prompt, so that the users don't have to use comments.
karthink
commented
3 months ago In my opinion, instead of using comments, we could embed those overlays in the buffer, and when the region is selected, those overlays will be baked into the prompt, so that the users don't have to use comments.
This is a good idea, but it will make the full-prompt construction via buffer parsing quite tricky. Also note that each backend-type has a custom parser for constructing the prompt. The overlay state management is going to be full of edge cases too. I'd say it's out of scope for now, using comments works fine. The additional directive is just a way to specify instructions on top of the system prompt for the next request.
As for why you don't see them, there appears to be a bug -- it's at the top of your transient menu instead of over the content in the main buffer. I'll check if I can reproduce this bug in a sandboxed environment now.
karthink
commented
3 months ago Yup, I can reproduce the bug, fixed.
karthink
commented
3 months ago Just a note that I'm going to be stepping away from gptel for a few days -- I've added/changed a whole bunch of things in the past two weeks, so I want to wait for things to stabilize and any bugs to be ironed out before continuing to add features.
Feel free to create issues if you encounter any more bugs -- I will be keeping an eye on the tracker.
munen
commented
2 months ago Coming here to give a :+1: for adding a mechanism to add context to the prompt.
In the last year, I have tried various AI integrations for Emacs, the commandline and even VSC to see what other platforms are handling this. gptel is a clear winner in all aspects to me with one thing missing: Having the ability to add relevant regions to the context for the query. I already started to look into how I could build this myself and then found this PR.
I do like this PR, however, I think I know why it hasn't been merged as of now. It does multiple things:
In my opinion, 1 is adding great value and is quite simple. As a programmer myself, I do see the value of 2, of course. However, it is a more specific use case and if I understand karthink correctly, he wants to keep the upstream implementation very clean and simple.
karthink
commented
2 months ago In the last year, I have tried various AI integrations for Emacs, the commandline and even VSC to see what other platforms are handling this. gptel is a clear winner in all aspects to me with one thing missing
I haven't looked around to see what other LLM clients offer since ~May 2023. Surely someone must have written something more featureful and seamless than gptel? A quick google search indicates that there are multiple (paid) commercial offerings for VSCode, I can't imagine them doing worse than gptel. This package is just a fancy wrapper around Curl.
I think I know why it hasn't been merged as of now. It does multiple things:
The main reason is that gptel is a one-weekend-per-month project for me and that time is eaten up fixing bugs! There are three features on the roadmap that I haven't had the time to work on: attaching context (this PR), supporting function calling, and multimodal support (mainly vision) in chats. Copilot-style completion-at-point, a fourth feature, seems very difficult to do via the Chat APIs so I've shelved that plan for now.
- Adds the ability to add multiple regions to the context.
- Adds a mechanism for refactoring using specific comments
In my opinion, 1 is adding great value and is quite simple.
This is correct! 1 is definitely on my TODO list. I don't want to add 2 to gptel but I want to make it easy for users to build workflows like that on top of 1. I don't yet know what the best way of doing this is, and haven't had time to write some prototypes.
I understand karthink correctly, he wants to keep the upstream implementation very clean and simple.
Yes, keeping gptel simple and focused is the only way for it to remain maintainable given my time constraints.
@munen Love your Emacs work and presentations! Thank you for organice as well, it's very handy!
daedsidog
commented
2 months ago I too looked around other LLM clients, and, at least when it comes to programming, this method is the best way I found working with code. In most cases, I rarely type code anymore, but instead "think" of exactly how I want to implement something, then provide very precise instructions. 95% of the time, GPTel does exactly what I want it to do with the instruction comments.
Another useful feature of the contexter is even outside code. I mark regions in other buffers, open the context buffer, then paste the already Markdown-formatted buffer directly to the dedicated chat buffer, then reason around with what I want with the model. Saves a lot of time.
I did have a few "ambitious" prototypes before settling on manual marking of context. My first try was completely contextless, where the user just provides the instruction comment, then prompts the AI to give a sequence of Emacs commands to get the relevant context. As you may already guess, it was pretty bad, so this is the next best thing. I found it very useful and wanted to share the concept, and don't mind if its integrated or not.
Obviously it is mostly a hack than a seamless feature implementation at this current stage, chiefly because there are many things which I myself am not sure how to integrate properly. For example: right now to generate the prompt, I insert the entire context implace in the buffer, then use GPTel's own buffer refactoring function instead of calling the API directly. It was obvious to me from the very beginning that this would not be merged as is.
I am willing to work on it to specification if I am given instructions, though.
karthink
commented
1 month ago I am willing to work on it to specification if I am given instructions, though.
Here's what I have in mind:
The context list is a list of buffers, files and references to regions specified by the user.
Binary files are not supported for now, but will be when I add support for multi-modal models.
Its scope is global in Emacs, but can be set buffer-locally if required.
When building the query to be sent, we check the context list. If it is not empty, we run through it, collecting the text of the regions, buffers and files, and either appending it to the system prompt, or prepending it to the first user message.
The query can then be inspected using the dry-run options, or sent as usual. (I need to make the dry-run/inspection options more accessible to avoid confusion.)
This means you cannot prepare and edit a "context buffer" where you can tweak the context, since the context text is only built at send-time. This is intentional.
Maintaining a context list instead of context text and building it at send-time also means it's never going to be out of date.
A gptel-add command that works in a DWIM fashion:
Regular call: Add current buffer to the context list
Call with prefix-arg: Read buffer name to add to the context list
Call with negative prefix-arg: Remove current buffer from context list
Call with region active: Add region to context list
Call in dired: Add marked files or file at point to context list
More focused gptel-add-file, gptel-add-buffer, gptel-add-region commands
gptel-menuI don't know yet if the context menu should be a separate sub-menu or if it can fit in the main menu.
daedsidog
commented
1 month ago Its scope is global in Emacs, but can be set buffer-locally if required.
It's difficult for me to think about local context buffers, since (right now) the process of adding context is going to another buffer and marking different regions that are appended to the context list. How would buffer-local context lists work?
This means you cannot prepare and edit a "context buffer" where you can tweak the context, since the context text is only built at send-time. This is intentional.
Just to make things clear: even my current method does not involve preparing a context buffer. The context buffer is there to visualize the context list, and allow for easy removal of contexts. It is also generated on-demand, and cannot be edited manually, making the context always be updated. Updating the region in some buffer which appears in the context buffer will result in an instantly updated context buffer.
Call with negative prefix-arg: Remove current buffer from context list
Right now, I have a command where if a regular call to context-adding is made while the point is within a context region, the context region gets removed. I think it would be preferable that instead of a negative prefix-arg. I find this more ergonomic.
I would suggest that a wrapper function, gptel-add-or-toggle-context, that would call gptel-add, based on your specification, while also doing what I just mentioned.
- Show the context list, truncate if necessary
I think this is unnecessary, as contexts are generally very long and not user-informative. I don't see how it can be fit in the gptel-menu. Instead I recommend a menu that adds an option to display the contexts, which opens a transient buffer similar to how you can edit the system message now. In that buffer, you can view the context, & should be able to selectively remove context from there.
Some other points:
In buffer `c-buffer`:
```c
int a, b, c;
```
In buffer `some-buffer`:
```
aaaa
bbbbbbb
```
In buffer `*scratch*`:
```
context a
context b
```Should this be changed? Should this be user specified? Should this be optional?
karthink
commented
1 month ago It's difficult for me to think about local context buffers, since (right now) the process of adding context is going to another buffer and marking different regions that are appended to the context list. How would buffer-local context lists work?
It can only work when adding buffers or files from the transient menu, where you pick them using completing-read. When using gptel-add, the scope will always be global.
Just to make things clear: even my current method does not involve preparing a context buffer. The context buffer is there to visualize the context list, and allow for easy removal of contexts. It is also generated on-demand, and cannot be edited manually, making the context always be updated.
Yes, this is the behavior we'd like.
Right now, I have a command where if a regular call to context-adding is made while the point is within a context region, the context region gets removed. I think it would be preferable that instead of a negative prefix-arg.
Good idea!
I think this is unnecessary, as contexts are generally very long and not user-informative. I don't see how it can be fit in the
gptel-menu.
I don't follow. A truncated list of buffer names in the menu is better than no indication that additional context is active.
Instead I recommend a menu that adds an option to display the contexts, which opens a transient buffer similar to how you can edit the system message now. In that buffer, you can view the context, & should be able to selectively remove context from there.
I think region contexts should be highlighted in the buffers, it helps to know what is part of a context and what isn't.
Right now, when I want to send the context list to the GPTel call, I surround the context snippets with useful metadata (what line they appear in, what file, etc.) This formatting is done with markdown.
Yes, all three of these are planned. I'm not sure about exactly how to annotate the snippets (last point), markdown blocks are one option.
Should this be changed? Should this be user specified? Should this be optional?
Right now it doesn't need to be customizable.
karthink
commented
1 month ago @daedsidog I took a detailed look at your code in gptel-contexter.el. It looks good, here are a few comments from the perspective of adding it to gptel:
The bookkeeping to handle regions can be simplified considerably, since overlays contain buffer and marker information to keep track of them. Other functions, like gptel-context-in-region and gptel-context-at-point can be much simpler as well - we can just check overlays-at. This is the case for display behavior in the context buffer as well (gptel--highlight-selected-context-in-context-buffer).
Live updates: I don't want to add anything to after-change-functions, which means no live updates in the context buffer. after-change-functions is a landmine, it is difficult not to slow down Emacs because of interactions between an after-change function, other minor modes in the buffer and buffer edits. We can set an auto-revert-handler for the context preview buffer and bind it to g or something.
I don't want to add to post-command-hook or kill-buffer-hook globally either. post-command-hook is very busy, and the chief cause of poor Emacs performance in my experience.
Extra stuff: You have additional features (in gptel--compress-code, gptel-context-substring etc) that are not planned for gptel.
Overall, I'd say it's a considerable amount of work to adapt this to gptel, and you'd have to separate the extra features that are out of the scope of the PR and find a way to plug them back in from your personal configuration.
It's probably easier to write a simple prototype from scratch, using gptel-contexter.el as inspiration, than it is to hack away at it. I can write the prototype using this PR as inspiration, I don't think it will take long. Does that work for you, or are you up for reducing the scope of gptel-contexter and rewriting a big chunk of the bookkeeping?
daedsidog
commented
1 month ago I don't follow. A truncated list of buffer names in the menu is better than no indication that additional context is active.
I understand what you meant now.
It's probably easier to write a simple prototype from scratch, using gptel-contexter.el as inspiration, than it is to hack away at it. I can write the prototype using this PR as inspiration, I don't think it will take long. Does that work for you, or are you up for reducing the scope of gptel-contexter and rewriting a big chunk of the bookkeeping?
I think I would prefer to hack away at it, removing all the extras to my config.
One thing I'm unsure of is how to reduce the bookkeeping, do we not want to keep track of all the contexts? When buffers are killed, do we not want to make sure that we know those contexts are dead? I know of no other way. Could you expand a little on that? I'm not very knowledge in the behind-the-scenes mechanisms and performance blunders of Emacs, so I apologize in advance & appreciate the direction in these areas.
Upon rereading, I think I understand how to simplify it:
The bookkeeping to handle regions can be simplified considerably, since overlays contain buffer and marker information to keep track of them. Other functions, like gptel-context-in-region and gptel-context-at-point can be much simpler as well - we can just check overlays-at. This is the case for display behavior in the context buffer as well (gptel--highlight-selected-context-in-context-buffer).
So basically, since Emacs tracks the overlays, we could just generate everything on-demand by going through them all, with no need to enumerate them. Okay.
karthink
commented
1 month ago I think I would prefer to hack away at it, removing all the extras to my config.
Okay. I actually have a minimal version of gptel-context.el written already (it only took an hour), but I'll wait for updates to gptel-contexter.el instead then.
One thing I'm unsure of is how to reduce the bookkeeping, do we not want to keep track of all the contexts? When buffers are killed, do we not want to make sure that we know those contexts are dead? I know of no other way. Could you expand a little on that?
Sure. We want to use a "pull" method instead of a "push" method to keep track of state, due to Emacs' rather low performance ceiling. The proposed read-only *gptel-context* preview buffer does not auto-update, the user presses g (say) to refresh/revert it, like you do dired or magit buffers. This means the contents of *gptel-context* can be out of date. When you revert the buffer, we run through the list of context sources and rebuild the context buffer. At this time we can throw away dead buffers, deleted overlays and nonexistent or unreadable files.
So basically, since Emacs tracks the overlays, we could just generate everything on-demand by going through them all, with no need to enumerate them.
We still have to keep track of the overlays, but the overlays will keep track of the corresponding buffers and markers etc. Assuming you create the overlay as (setq ov (make-overlay beg end nil 'front-advance)), we can use the fact that the overlay data structure contains the following fields:
(overlay-buffer ov)(overlay-start ov) and (overlay-end ov). You don't need to add extra markers and keep track of them.(overlay-put ov 'gptel-context t), and later (overlay-get ov 'gptel-context).So gptel--context-alist doesn't need to be an alist, just a list renamed to gptel--context, say. Each entry in the list is either an overlay, a buffer or a file-name (string), and we can dispatch on the type with cl-typecase.
Checking if point is inside a gptel context region is actually even simpler, it's just (get-char-property (point) 'gptel-context), or (get-char-property-and-overlay (point) 'gptel-context).
I'm not very knowledge in the behind-the-scenes mechanisms and performance blunders of Emacs, so I apologize in advance & appreciate the direction in these areas.
No worries. Basically anything you add in a hook that is (i) global and/or (ii) runs all the time, like post-command-hook, post-self-insert-hook or after-change-functions, will eventually bring Emacs to a halt. It will usually be due to some non-obvious interaction with behavior provided by other packages that the user has installed that we cannot anticipate. (after-change-functions is particularly insidious -- it runs even if some text properties in the buffer change, with no change to the buffer contents.)
Even well-tested and innocent built-ins like show-paren-mode (which adds to post-command-hook) cause performance problems when, for example, multiple cursors are involved, or if you customize jit-locking. This is a pain to debug, since we'll have forgotten by that point that gptel has installed these hooks. After decades of fighting with Emacs, I avoid packages that install more hooks than they absolutely need to, and prefer "pull" behavior (initiated by the user) instead of "push" behavior (from event-driven hooks).
The downside is that we can't have the nice auto-updating behavior of the kind you use in gptel-contexter.el, where the context preview buffer is always in sync. Until Emacs gets support for background threads, I'm not going to use them and don't want to include this behavior in my packages.
daedsidog
commented
1 month ago Thanks. That answers all the unknowns I had. I'll work towards that.
karthink
commented
1 month ago I've created a feature-context branch in this repo, please switch the PR destination to this branch? I hope that's possible to do from the Github UI without having to create a new PR.
daedsidog
commented
1 month ago I've created a feature-context branch in this repo, please switch the PR destination to this branch? I hope that's possible to do from the Github UI without having to create a new PR.
Doesn't look like it's possible (or I at least don't see a way). I can make a new PR, if that's the case. EDIT: Nevermind!
karthink
commented
1 month ago We can do it later, before merging.
daedsidog
commented
1 month ago @karthink I've reformed the context adding & sampling. Would you be willing to add some sort of skeleton to the transient UI that I can work from? I don't want to disturb it beyond what you deem necessary. Alternatively, draw something rudimentary? IMO there should be just two additions to the transient menu: a display that shows the number of contexts/the affected buffers, etc., and another menu which opens up the context buffer, which allows for quick and easy removal of existing contexts from within it.
daedsidog
commented
4 weeks ago @karthink This PR is nearly complete. You can ignore my previous message as I already took the liberty of adding a transient menu of my own flavor.
I've done most of what you suggested, except for this:
Call in dired: Add marked files or file at point to context list
I feel like there are cases where I would prefer to use the dired directory structure as context in itself, so I chose not to implement this for now.
Regarding everything else, so far I find it satisfactory. I feel that it falls in line with the spirit of this project. You be the judge of that:
The transient menu is only available with expert commands for now.
There are no hooks other than the buffer-local one responsible for monitoring the change in the point position to highlight underlying contexts in the contexts buffer.
I am doing the final steps of actually injecting the context where it is supposed to go, but thought to update the progress just in case there should be any changes made to the layout of the transient menu or other features.
daedsidog
commented
4 weeks ago @karthink I think the current state is satisfactory.
gptel-context-preamble and gptel-context-postamble custom variables so that users may bind them for extra instructions given the context.:nowhere, :before-user-prompt, :after-user-prompt, :before-system-message, :after-system-message. I've tested that all these work, but haven't played around to see whats better for what model. gptel-context-injection-destination is what uses these values to control the aforementioned behavior. gptel-context-highlight-face.gptel-use-context-in-chat.karthink
commented
3 weeks ago @daedsidog Thank you, I will review this soon. I just brought the feature-context branch up to master, could you rebase and resolve the conflicts?
karthink
commented
3 weeks ago A few preliminary comments before doing a full review:
One thing I'm unsure of is how to reduce the bookkeeping, do we not want to keep track of all the contexts? When buffers are killed, do we not want to make sure that we know those contexts are dead? I know of no other way. Could you expand a little on that?
Sure. We want to use a "pull" method instead of a "push" method to keep track of state, due to Emacs' rather low performance ceiling. The proposed read-only gptel-context preview buffer does not auto-update, the user presses g (say) to refresh/revert it, like you do dired or magit buffers. This means the contents of gptel-context can be out of date. When you revert the buffer, we run through the list of context sources and rebuild the context buffer. At this time we can throw away dead buffers, deleted overlays and nonexistent or unreadable files.
So basically, since Emacs tracks the overlays, we could just generate everything on-demand by going through them all, with no need to enumerate them.
We still have to keep track of the overlays, but the overlays will keep track of the corresponding buffers and markers etc. Assuming you create the overlay as (setq ov (make-overlay beg end nil 'front-advance)), we can use the fact that the overlay data structure contains the following fields:
The better way to do this is to maintain a gptel-contexts or gptel-context-overlays list. Then no scanning is required.
We need a better approach for attaching the context string to user messages -- right now you do it in gptel--parse-buffer in gptel-openai. Under this design it will have to be done separately for each backend (gptel-ollama, gptel-anthropic etc).
Attaching context to user messages (as opposed to the system message) also has to account for the recently added gptel-track-response, which see.
Please remove gptel-clean-up-llm-code from gptel.el, as it's unrelated to this PR.
daedsidog
commented
3 weeks ago We need a better approach for attaching the context string to user messages -- right now you do it in gptel--parse-buffer in gptel-openai. Under this design it will have to be done separately for each backend (gptel-ollama, gptel-anthropic etc).
I was aware of this. Different backends will have different data formats, so I went with this approach. Not sure how to do it differently right now, as I don't have any guarantees about other backends.
I'll handle the other bullets. Thank you.
karthink
commented
3 weeks ago I was aware of this. Different backends will have different data formats, so I went with this approach. Not sure how to do it differently right now, as I don't have any guarantees about other backends.
I'll look into doing this at a higher level in gptel to try and avoid backend-specific code.
karthink
commented
2 weeks ago The function for handling deletions in the context buffer is quite messy, I'll rewrite that too.
karthink
commented
2 weeks ago I've simplified gptel--suffix-context-buffer significantly, you can take a look and try it out now. No text properties are in use right now. Most of the behaviors from before should work the same. One change is that there's one block per context chunk, I'm not combining different context chunks from the same buffer into one block.
I haven't touched gptel-context-string yet, I'll get to it. Before that, I think the context buffer has enough special behaviors that it makes sense to just make it a major-mode, something like gptel-context-buffer-mode. I'm doing that now.
daedsidog
commented
2 weeks ago One change is that there's one block per context chunk, I'm not combining different context chunks from the same buffer into one block.
What you have right now for the context buffer looks much cleaner than what was previous there for the user. Since this doesn't actually get passed down to the model, this is preferable.
However, I recommend against such a drastic change in gptel-context-string, because this is what actually gets passed down to the model as data, and I found the correct formatting to work well. Down the line though, you would probably want the format of the context string decorations to be customizable.
For the context:
The context buffer looks like this:
Data passed to the model (through gptel-context-string) looks like this:
I suggest some more user-friendly formatting to be added (line numbers?) but otherwise, it looks much better this way.
Thanks for the simplifications! They're indeed much better.
karthink
commented
2 weeks ago However, I recommend against such a drastic change in gptel-context-string, because this is what actually gets passed down to the model as data, and I found the correct formatting to work well. Down the line though, you would probably want the format of the context string decorations to be customizable.
I haven't made any change to gptel-context-string yet. For now I only plan to change the mechanism for passing the information around, not the content of the string itself.
In the future, I plan to add ways for context to be collected automatically, and the UI and introspection tools for this needs to be simple/universal enough to allow for this.
karthink
commented
2 weeks ago Added gptel-context-buffer-mode, along with a new command gptel-context-visit, bound to RET in the context buffer.
karthink
commented
2 weeks ago Data passed to the model (through
gptel-context-string) looks like this:
One concern I have is about sending file names -- it's not obvious to the user that the full path of the file is going to be sent. This might have private information, such as the username, project name, hierarchy etc.
daedsidog
commented
2 weeks ago One concern I have is about sending file names -- it's not obvious to the user that the full path of the file is going to be sent. This might have private information, such as the username, project name, hierarchy etc.
In retrospect, I'm pretty certain this should never happen anyway. I remember that the full path is used if the context snippet has no associated buffer with it, which should never happen now. Not sure why the full path appears in that image, it should be using the buffer name.
EDIT: I had it mixed the other way around: It should default to filenames and then default to buffer name if no file associated with the buffer exist. I'll remove the file paths entirely.
karthink
commented
2 weeks ago One concern I have is about sending file names -- it's not obvious to the user that the full path of the file is going to be sent. This might have private information, such as the username, project name, hierarchy etc.
In retrospect, I'm pretty certain this should never happen anyway. I remember that the full path is used if the context snippet has no associated buffer with it, which should never happen now.
It happens because you're using buffer-file-name:
(setq buffer-file
;; Use file path if buffer has one, otherwise use its regular name.
(if (buffer-file-name buffer)
(format "`%s`"
(buffer-file-name buffer))
(format "buffer `%s`"
(buffer-name buffer))))buffer-file-name will return the full path of the file displayed in the buffer.
daedsidog
commented
2 weeks ago It happens because you're using
buffer-file-name
You beat me to it. I'll change that.
karthink
commented
2 weeks ago It happens because you're using
buffer-file-nameYou beat me to it. I'll change that.
Okay. I'm continuing to work on the PR, should I pause it until your next commit?
I see your commit, thank you.
karthink
commented
2 weeks ago Following the discussion on using the "pull" instead of "push" method to track context chunks, I'm removing gptel-contexts-in-region in favor of just checking gptel--context-overlays instead, with similar changes to other parts of the code where buffers are scanned for overlays.
karthink
commented
2 weeks ago I'm a little confused about why you're propertizing the context chunks. In gptel-buffer-context-string you have
(put-text-property 0 (length substring)
'gptel-context-overlay
context substring)And in gptel-context-string you have a function argument to leave the context chunks propertized. But this is not used anywhere now. I'm guessing this was used only to construct the context buffer via the transient menu?
karthink
commented
2 weeks ago I've simplified the context string creation, and also made it a little more efficient (mostly by consing less).
The contents of the context string are unchanged.
Context overlay tracking is now done with gptel--context-overlay-alist, which is an alist mapping buffers to context overlays in those buffers. Organizing this list by buffer (instead of using a flat list) helps in many places. We have to sort by buffer to construct the context string anyway.
karthink
commented
2 weeks ago It's possible I might have introduced some bugs. Please test.
The next steps for this PR are the following:
gptel--parse-buffer for all supported LLM APIs.gptel-add-context is run the first time, or when gptel--suffix-context-buffer is called. Some combination of declare-function, autoloads and require should do the job.gptel-context prefix, and make most noninteractive functions internal.karthink
commented
2 weeks ago @daedsidog You appear to have some changes in this branch that are unrelated to the PR, involving gptel--rewrite-menu and gptel-rewrite-message. I plan to rebase this branch onto feature-context and will be overwriting these changes via a force push. Are you okay with that?
I will also eventually squash the commits into a small set of meaningful ones, maybe 3-4 commits, before merging. Right now most of these commits are hard to follow.
daedsidog
commented
2 weeks ago I'm a little confused about why you're propertizing the context chunks. In
gptel-buffer-context-stringyou have(put-text-property 0 (length substring) 'gptel-context-overlay context substring)And in
gptel-context-stringyou have a function argument to leave the context chunks propertized. But this is not used anywhere now. I'm guessing this was used only to construct the context buffer via the transient menu?
I put the property there to mark the content of the context inside the entire string, so that I can fetch it out of the decorations. I used it to determine which part to highlight when generating the context buffer. Now that it's refactored, & I can't think of any user-side usage of it, I think there is no longer any need to propertize at all.
@daedsidog You appear to have some changes in this branch that are unrelated to the PR, involving gptel--rewrite-menu and gptel-rewrite-message. I plan to rebase this branch onto feature-context and will be overwriting these changes via a force push. Are you okay with that?
I don't recall changing those. Since they are not germane, yes, go on ahead. Thank you.
karthink
commented
2 weeks ago Rebased onto feature-context, as there were several changes unrelated to the PR, as well as many merge conflicts. (I think your branch is based on an older version of gptel.)
karthink
commented
2 weeks ago Do we need both the :before-system-message and :after-system-message options? (Similarly both :before-user-prompt and :after-user-prompt.)
This level of granularity seems unnecessary, I'm not sure if it makes any difference where the context is relative to the rest of the system message or user prompt. Why not just
:with-system-message (append to system message),:with-user-prompt (prepend to user prompt):nowhere (or nil) value?This will simplify the configuration, and the bookkeeping in the code.
daedsidog
commented
2 weeks ago Do we need both the
:before-system-messageand:after-system-messageoptions? (Similarly both:before-user-promptand:after-user-prompt.)This level of granularity seems unnecessary, I'm not sure if it makes any difference where the context is relative to the rest of the system message or user prompt. Why not just
* `:with-system-message` (append to system message), * `:with-user-prompt` (prepend to user prompt) * and a `:nowhere` (or `nil`) value?This will simplify the configuration, and the bookkeeping in the code.
I would say yes, because the nature of the context is not immediately obvious. I would actually say that a more granular method is preferred. Right now, there are only two options (per destination), but I found myself sometimes needing things like having my message, context, then another message again, all in 1 prompt; my solution to this was adding the variables gptel-context-preamble and gptel-context-postable, but I don't like this.
I admit, however, that I do treat LLMs like I would treat a human reader, and try to form the clearest prompts possible, which is why the position of the context & their decorations mattered to me. This might not be necessary, but I don't truly know.
karthink
commented
2 weeks ago I would say yes, because the nature of the context is not immediately obvious. I would actually say that a more granular method is preferred. Right now, there are only two options (per destination), but I found myself sometimes needing things like having my message, context, then another message again, all in 1 prompt; my solution to this was adding the variables
gptel-context-preambleandgptel-context-postable, but I don't like this.
If you want a fine-grained arrangement of prompts like this, I think gptel's configuration is the wrong place to do it. Adding more levers here just makes using gptel more confusing, without ever achieving the kind of free-form arrangement you're looking for. The intended solution here would be to simply create a buffer and add everything you need manually before sending it, i.e. use Emacs' free-form editing capabilities.
So from a usability and maintenance perspective, I'm inclined to reduce the granularity here. Right now there are too many parts to even just the request's system message: the system message (from gptel-directives), the additional directive (from gptel-menu), the context preamble, the context chunks and the postamble. I expect users to find this very confusing.
I admit, however, that I do treat LLMs like I would treat a human reader, and try to form the clearest prompts possible, which is why the position of the context & their decorations mattered to me. This might not be necessary, but I don't truly know.
I don't know either, but it's a safe bet that this is what most users (including me) will want to do as well. This isn't possible with any predefined/pre-configured template though.
daedsidog
commented
2 weeks ago If you want a fine-grained arrangement of prompts like this, I think gptel's configuration is the wrong place to do it. Adding more levers here just makes using gptel more confusing, without ever achieving the kind of free-form arrangement you're looking for. The intended solution here would be to simply create a buffer and add everything you need manually before sending it, i.e. use Emacs' free-form editing capabilities.
I share this sentiment now. It never occurred to me to customize a buffer like this, but now I will definitely do this over haphazardly overwriting the dynamic variables when doing in-buffer rewrites.
The majority of the time that I used the context was not in in-buffer rewrites (the whole reason for the preamble/postamble mess) but to actually just build a list of things which I insert in the chat buffer, which come formatted in nice Markdown.
However, a major convenience was also having GPTel automatically embed the context in, say, the last prompt, without having to do it manually. What has been done so far was messy, but it was nice nonetheless. It would still be possible to replicate strictly with Emacs commands, but that would mean both levers (destination & usage in chat buffer) would become obsolete.
This adds the contexter I've been working on for my own personal use to GPTel. As of now, this pull request is not complete because I'm unsure what's the best path to integrate it, and also how it will fit with the new UI refactor. I synced up with the master branch of GPTel, and just added that in, but my transient menu seems to be vastly different from the previews I've seen.
Because this started as part of my config & grew organically, there are still some features that are part of my config and not in GPTel. Those parts are added to the end of this pull request in the form of a snippet from my config.
In a nutshell, I personally find this to be an incredibly useful programming tool for productivity, more so than any other AI programming tool I've tried. It pampered me to the point where I don't think I can go back to programming without it.
Here is a short guide how to use it with my config (this will change the more things I'll offload to GPTel):
C-c g p. You can presscto remove selected context snippets from the context buffer directly, orC-c g pagain to remove context snippets from the code buffers.C-c g rto send it to GPTel.With Lisp, the contexter automatically collapses s-expressions in areas that they are not required in, such as functions with docstrings, thus saving tokens:
Here is the part of my config file which sets all this up:
As can be plainly seen from the config file, the mechanism to actually send this to GPTel is rather primitive. It uses keyboard macros to navigate the transient menu, and then just dumps the prompt into the buffer, and uses the transient refactor menu to send it to GPTel.