-
Notifications
You must be signed in to change notification settings - Fork 237
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
Cleanup code context and code features #401
Conversation
…api, modify get code message
The LLM filter no longer knows which files are user included; how should we give it this information? I think just giving it all of the file names would work well, but let me know your ideas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a great PR, look at those numbers! +507 −732
In hindsight I really overdid it with the caching 😅
Lots of good refactors. I left a few suggestions, take em or leave em.
auto_tokens = min(get_max_tokens() - tokens_used, config.auto_tokens) | ||
|
||
# Get auto included features | ||
if config.auto_context and prompt: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if config.auto_context and prompt: | |
if config.auto_context and prompt and auto_tokens > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my persistent context PR I actually merge auto_context and auto_tokens into auto_context_tokens and I check if it's more than 0 there, so this change is pretty much already made!
|
||
async def get_code_message( | ||
self, | ||
prompt_tokens: int, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand the system ultimately goes back to model.config
. This will make it difficult to use with other models/scenarios, like what we did/plan-to-do with LLMFeatureFilter. Also, using max_tokens
seems like a cleaner interface for the function? Just "give me up to N tokens of context".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh our LLMFeatureFilter should have a test to catch that - making a note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to prompt_tokens instead of max_tokens because we don't actually tell it to give us up to N tokens of context; we tell it to include all of the user's include files (since we no longer truncate it) and give us up to auto_tokens of automatic context (as long as it fits). The prompt tokens is just so we can check if there are enough tokens for user included files, and so we can only add as many auto context files as can fit in context. I think this makes a lot more sense to me (since we aren't truncating user included files anymore).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by difficult to use with different models/scenarios? I'm not sure I understand
if ( | ||
section.start >= self.interval.start | ||
and section.start < self.interval.end | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( | |
section.start >= self.interval.start | |
and section.start < self.interval.end | |
): | |
if self.interval.intersects(section): |
DiffAnnotations inherit Interval so this works. Also, the earlier wouldn't catch an annotation spanning multiple intervals. Still not ideal, but will guarantee it's there at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually an intersection check; this is checking to ensure that the diff annotation starts within the interval (if it ends inside of the interval, we don't want to include it because a. it could be included in multiple intervals and b. the start of the diff annotation wouldn't make much sense because those line numbers aren't in this interval)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
99e4ab9
to
b5a882b
Compare
Nice work tracking it down! Much faster now. I guess this only matters if there are a lot of files with changes, but that should be super rare. I guess worst case someone is using Mentat to help them review a big PR that changes dozens of files? Perhaps we could call |
TODO: