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

Only refresh code context once a loop #527

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Only refresh code context once a loop #527

merged 3 commits into from
Feb 20, 2024

Conversation

jakethekoenig
Copy link
Member

@jakethekoenig jakethekoenig commented Feb 16, 2024

If multiple files are added or when checking the diff the code_context can end up being refreshed many times in one operation. With this change it should only be refreshed once at startup and at most once after user input.

On my computer with two unignored venvs this reduces start up time from 4.5 to 2.5 seconds. On a clean git project it doesn't have much effect.

I'm going to look for more wins. I think the start up time can be pushed even lower.

Pull Request Checklist

  • Documentation has been updated, or this change doesn't require that

If multiple files are added or when checking the diff the code_context
can end up being refreshed many times in one operation. With this change
it should only be refreshed once at startup and at most once after user
input.
@biobootloader
Copy link
Member

What if the user modifies files directly after Mentat has started? Does that require a refresh?

@PCSwingle
Copy link
Member

Whenever mentat creates, deletes, or renames a file it won't show up; ditto with running /undo or /redo with deletions creations and renames. Also, with auto context on, it doesn't refresh until the end of the response, so you don't get to see what it's working with while it's streaming. Once those things are fixed I think this should be good (although I might have missed some other cases)

@jakethekoenig
Copy link
Member Author

Whenever mentat creates, deletes, or renames a file it won't show up; ditto with running /undo or /redo with deletions creations and renames. Also, with auto context on, it doesn't refresh until the end of the response, so you don't get to see what it's working with while it's streaming. Once those things are fixed I think this should be good (although I might have missed some other cases)

In the first case I think it actually will because it's refreshed in the cost_handler for some reason. I think to handle this case. Honestly might be best to put it in the main loop. Though that's weird because commands/prompts are handled differently

@PCSwingle
Copy link
Member

PCSwingle commented Feb 17, 2024

In the first case I think it actually will because it's refreshed in the cost_handler for some reason. I think to handle this case. Honestly might be best to put it in the main loop. Though that's weird because commands/prompts are handled differently

It's refreshed in the cost handler because the context also shows the total cost and that needs to get updated. It also doesn't work with mentat changing files because mentat doesn't actually change the file until the user accepts with Y (but the refresh is done right after the call is finished). I think putting it in the main loop actually makes a lot of sense. We could also probably remove it from the cost handler then.

@jakethekoenig
Copy link
Member Author

In the first case I think it actually will because it's refreshed in the cost_handler for some reason. I think to handle this case. Honestly might be best to put it in the main loop. Though that's weird because commands/prompts are handled differently

It's refreshed in the cost handler because the context also shows the total cost and that needs to get updated. It also doesn't work with mentat changing files because mentat doesn't actually change the file until the user accepts with Y (but the refresh is done right after the call is finished). I think putting it in the main loop actually makes a lot of sense. We could also probably remove it from the cost handler then.

I'll sleep on it and make a better pr Monday. Seeing the order of execution across the context/conversation files makes me think greater improvement in both efficiency and clarity are possible

@jakethekoenig
Copy link
Member Author

Sorry for the delay on this. I kept noticing things I wanted to change but decided I needed to split things and still wanted to make this change. I think this change stands alone as an improvement though and can be merged as is.

Other things I want to do:

  1. Move the responsibility for code_message to conversation, have it provide a dedicated method for getting token counts. In a few places callers ask for the messages just to generate the code message just to count the tokens.
  2. Pass the --directories flag to the git method in charge of getting untracked files. That way even if node_modules is unignored the git call will be fast.
  3. Render untracked and files with diffs differently on the frontend.
  4. There is a lot of logic in get_code_message that is only relevant if auto_context is on. It should be moved into the if statement for a slight speed up.
  5. We recompute token counts for messages. We could make a wrapper Message class that provides a cached token_count() method.
  6. diff_context is None if it's not in a git repo. This makes interacting with it more complex. I think it would be simpler if it was always defined but its functions returned sensible none types when not in a git project. e.g. diff_files returns [].

Copy link
Member

@PCSwingle PCSwingle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@jakethekoenig jakethekoenig merged commit a2d109f into main Feb 20, 2024
16 checks passed
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.

3 participants