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

fix(agent): normalize indentation in code completion postprocess #3361

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

Sma1lboy
Copy link
Collaborator

@Sma1lboy Sma1lboy commented Nov 2, 2024

#3263

Fixing wrong indent in inline completion predict solution

Demo

Screen.Recording.2024-11-15.at.09.24.21.mov

Before

Only first line has extra spaces:
image

exactly one extra space in the line indent
image

After

image
exactly one extra space in the line indent
image

Implementation

Using whether the current indent is even as the judgment basis. If it's even, the prediction doesn't need any extra spaces and the prediction context will be processed accordingly.

Potential risks:

Some users might use odd indent size

Why

This approach is simple and doesn't require adding additional LSP or obtaining the best indent through client operations.

Issue didn't fix

Due to some concern this post-processing not going to fix this situation

All lines in prediction have extra spaces:
image

@wsxiaoys wsxiaoys requested a review from icycodes November 2, 2024 04:01
@Sma1lboy
Copy link
Collaborator Author

Sma1lboy commented Nov 4, 2024

Follow up: This is kind of a post-processing filter to prevent inconsistent extra spaces due to different model capabilities. The above bad example happens when using the demo website model, but sometimes works fine with StarCoder-1B without this modification.

@icycodes icycodes changed the title fix: normalize indentation in code completion postprocess fix(agent): normalize indentation in code completion postprocess Nov 14, 2024
@icycodes
Copy link
Member

As all bad cases in #3263 have exactly one extra space in the line indent, I suggest we should only process the case of exactly one extra space in the line indent, not extra indent.
It should be allowed to have extra indents in the completion text. Consider the case of

function foo() {
║
}

and the completion is

function foo() {
├    bar();┤
}

@Sma1lboy
Copy link
Collaborator Author

As all bad cases in #3263 have exactly one extra space in the line indent, I suggest we should only process the case of exactly one extra space in the line indent, not extra indent. It should be allowed to have extra indents in the completion text. Consider the case of

function foo() {
║
}

and the completion is

function foo() {
├    bar();┤
}

The current version should work fine with this.

@Sma1lboy Sma1lboy requested a review from icycodes November 15, 2024 17:34
@icycodes icycodes merged commit 90b4b4c into TabbyML:main Nov 22, 2024
4 checks passed
@sboys3
Copy link

sboys3 commented Jan 26, 2025

This merged pull request mangles auto-completions when tabs are used instead of spaces.

It replaces parts of correctly tabbed responses with incorrect amounts of spaces. This has been driving me insane for a while, and today after reviewing completion logs I realized it was entirely client side. I had thought it was due to a mix of spaced type definitions and tabbed code when using javascript, but when I also saw it in a small c++ project, I looked into it deeper.

As a quick test I replaced return item.withText(normalizedLines.join("")); with return item; in the agent's bundle in my installed extension, and the problem completely went away with this code bypassed.
To do that in the current production bundle located at %userprofile%.vscode\extensions\tabbyml.vscode-tabby-1.18.0\dist\tabby-agent\node\index.js replace e.withText(n.join("")) with e/*indent fix*/

Here is an example of what has been happening. It has been extremely frustrating having to constantly fix indention.
Screenshot 2025-01-25 185534
Screenshot 2025-01-25 190639

And here is the result with this pull request bypassed
image

@Sma1lboy
Copy link
Collaborator Author

@sboys3 I am very sorry for causing this problem, I will fix the bug caused by this PR as quickly as possible

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