⚠️ No Changeset found

Latest commit: 940c543

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

The changes in this pull request seem to be related to enhancing the file reading capabilities and updating the tool usage format. The changes across different files appear to be interconnected, focusing on a single feature enhancement. Therefore, it doesn't seem necessary to split the pull request into smaller ones. If you have any questions or need further clarification, feel free to reach out!

• please convert this PR to a draft so it does not get merged accidentally
• @cte this will need run through an eval before merging, but I think it has great promise to reduce token usage (ie, the n^2/2 for sequential file reads), and increase focus because all the information is provided through a single request.

@KJ7LNW converted, @mrubens @cte we need an threshold of maximum file reading, should we based on model information for threshold ?

@KJ7LNW converted

Excellent please force push so I can look when you get a chance.

we need an threshold of maximum file reading, should we based on model information for threshold ?

I think first pass should respect existing line limit handling per file to keep the implementation simple . After we have been using it for a while we may discover different ways to handle length issues.

Please make sure that "list code definitions" triggers when the length of the file is less than the maximum line limit as it does now

@KJ7LNW i just update the test, now it all pass

<args>
:path:src/app.ts
:start_line:1
:end_line:50
======+++======
:path:src/utils.ts
:start_line:100
:end_line:150
======+++======
:path:package.json
</args>

It looks like you have custom argument formats which do not allow us to programmatically change the parser or automate tool XML structure integration as part of the tool refactor that @bramburn is working on in #2467 . Ultimately all tools will provide standardized argument structure so that no tool has special parsing for trivial data types (int, string, etc). The long term goal is to automatically generate system instructions, parsing, and tool documentation based on per-tool JSON-looking XML schema definitions standardized in a static attribute for each tool class.

Please choose a standard XML format, so we can standardize all tool arguments as part of the Tool base class in the future and avoid much fine your code to conform. Something like this would be appropriate:

<read_file>
<file path="/path1">
<range lines="1-100"/>
<range lines="210-22"/>
</file>
<file path="/path2">
<range lines="11-111"/>
<range lines="222-333"/>
</file>
</read_file>

or if you do not like attributes:

<read_file>
<file>
<path>/path/to/file1</path>
<line_range>111-222</line_range>
<line_range>333-444</line_range>
</file>
<file>
<path>/path/to/file2</path>
<line_range>1111-2222</line_range>
<line_range>3333-4444</line_range>
</file>
</read_file>

@KJ7LNW if we have format like that another llm can do it ?
I will make another branch for xml style

if we have format like that another llm can do it ?

LLMs work quite well with XML at the moment, I am not sure if I understood your question, can you elaborate?

@KJ7LNW i make it work with all xml, fixing test then push to branch 💪🏻

💪🏻!!!

I'll check it out

@mrubens @cte @KJ7LNW i just updated to all xml

the code looks good, I am going to try it out in real life and see what I discover ...

FYI, I have been running this on my desk for a couple of weeks for everything that I do, this is the feedback of my experience:

Performance issues to monitor:

* If users report slowdowns when reading 10+ files at once, we might need to consider adding concurrency limiting

I have not found any slowdown, and I have had times where 15-20 files were loaded at once. it is probably 10x faster than loading them individually.

* Watch for memory spikes - the current implementation loads all files into memory simultaneously

Fortunately, this happens on the backend, not in the web view.

* Keep an eye on VS Code becoming unresponsive during large batch reads

I have never seen that problem it has been nice and snappy.

Things that might break:

* Very large files (100MB+) being read in batches could cause OOM errors

That is an issue for single file reads, too, because a file that big would exceed available context for all existing models today; if you are using read-size limits, then this is not an issue. Read limits default to 500 lines, but it may have changed recently.

* No validation on total bytes being read - someone could accidentally read gigabytes (while this is also a potential issue with our current `read_file`, this implementation makes it easier for this to happen)

If this is going to be addressed it needs to be addressed per-file. We have line-limit constraints available to handle this already there is probably not a better way to do it.

Quick fixes to consider post-merge:

1. Add a simple concurrency limit (even just 5 concurrent file ops would help)

There already is. I keep mine at 100 and have never had a problem:

2. Track total bytes being read and warn/limit if it's too much

I disagree with this because we already have line limit handling that users can enforce. "too much" is too subjective, it will very by user model and use case so let's keep the existing line-limit behavior.

3. Make the error messages clearer when batch operations fail

is there a specific error message you are concerned about?

4. Add telemetry to see how people actually use this feature

Good idea.

Can this be merged and add telemetry in a separate PR?

When checked for the first time this should probably default to a higher number?

@samhvw8 can you set this to 15 by default immediately when experimental flag is enabled? Users can then adjust it from there.

@KJ7LNW, Can you check again the default value for concurrent reads? I made a change regarding that specifically.

The value is set to 15 for me when I enable the feature.

@KJ7LNW, Can you check again the default value for concurrent reads? I made a change regarding that specifically.

The value is set to 15 for me when I enable the feature.

If that is what it does for you, then it must be correct, as I am going off slightly older information.

sorry for any confusion