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

Add better default context string #349

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daedsidog
Copy link
Contributor

@daedsidog daedsidog commented Jul 26, 2024

While gptel provides the tools to make any arbitrary context format, I think it's important to have a very general and all-encompassing default one for new users.

I find that the old one was too confusing for many models and a lot of the times they included the line information and ellipses in the result, simply because those were embedded within the code block.

This patch makes the context string slightly more general and removes any embedded information that might be confusing to the models while still providing as much context as possible.

image

Also adds support for context with nested Markdown backticks.

@karthink
Copy link
Owner

Thanks. From a cursory look at the changes I don't understand what it's doing. I'll take a detailed look later, I'm pretty sure you don't need all all those flags and checks to do what you want.

@karthink
Copy link
Owner

Also is the column number information ever useful?

@daedsidog
Copy link
Contributor Author

daedsidog commented Jul 30, 2024

Also is the column number information ever useful?

It's only there if the context is actually not from the start of the line. I imagine that it could be confusing if you select multiple contexts from the same line and it would show the same line number (confusing for the model, not the user). Not that it wouldn't be able to deal with it otherwise, but at the same time I didn't really think it would hurt.

I have no real way to gauge usefulness to model beyond that.

I do know however that having non-embedded line numbers is vastly better, because now the models stopped returning them in their results. The new string is better than the old.

Thanks. From a cursory look at the changes I don't understand what it's doing. I'll take a detailed look later, I'm pretty sure you don't need all all those flags and checks to do what you want.

Feel free to simplify it. Mainly all it's doing is checking if the context is such that it should include the column number. There are also checks to see if the edges of the context are empty lines, and if so, move the position of the context edge in such a manner as to not include them.

Additionally, it also checks for nested markdown code blocks. I think this is useful for the model, but that's an anecdotal saying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants