-
Notifications
You must be signed in to change notification settings - Fork 291
fix(/context): gracefully handle huge context files + ux #1331
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
fix(/context): gracefully handle huge context files + ux #1331
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1331 +/- ##
=======================================
Coverage 16.75% 16.75%
=======================================
Files 213 213
Lines 20704 20704
Branches 871 871
=======================================
Hits 3468 3468
Misses 17236 17236 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9c96cc4
to
a49095c
Compare
caa85e5
to
c2e6d65
Compare
2b18018
to
43f9b4e
Compare
/// | ||
/// # Returns | ||
/// A Result containing a vector of (filename, content) pairs or an error | ||
pub async fn get_context_files(&self, force: bool) -> Result<Vec<(String, String)>> { | ||
pub async fn get_context_files(&self) -> Result<Vec<(String, String)>> { |
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.
also a note - from checking process_path
it seems like it's never invoked with force == true
so it would be nice to just remove that altogether
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.
Also a major TODO here is to limit the total size of files loaded. This whole context loading logic loads everything into memory before dropping, so it's super trivial to just run into OOM with a glob that matches a ton of large files.
/// sorted but the content will not be changed. | ||
/// | ||
/// Returns the dropped files | ||
pub fn drop_matched_context_files(files: &mut [(String, String)], limit: usize) -> Result<Vec<(String, String)>> { |
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.
An API that doesn't require copying potentially very large files would also be nice, just for memory performance (not like it matters even)
I would rather this func maybe take an iterator over files, and return a vec of dropped file names instead. Then, your files could be e.g. a HashMap
of file name to file content
43f9b4e
to
ead8f6a
Compare
ead8f6a
to
b1ac450
Compare
#1254
demo-context-file-exceeded-limit.mp4