-
Notifications
You must be signed in to change notification settings - Fork 435
fix(smart-apply): custom model now checks for token limit #7940
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
Open
ichim-david
wants to merge
8
commits into
main
Choose a base branch
from
ichimdav/smart_apply_cody_5727
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…smart apply The new Qwen based smart apply model performs an entire-file replace, so we can simply remove the leading newlines from the response. This simplifies the response transformation logic for this model since we no longer need todo extra formatting given the fact that we replace the entire file and it should be correctly formatted already
The new Qwen based smart apply model performs an entire-file replace, but we should only trim the final newline if the original file ends with a newline. This commit updates the response transformation logic for the new smart apply model to conditionally trim trailing whitespace based on whether the original file ends with a newline character. This ensures that files without a final newline are not incorrectly modified.
…odels This commit enhances the response transformation logic for the new Qwen-based smart apply model to provide more accurate and consistent results. Key changes: - Adds a `trimLLMNewlines` function to preserve or remove leading/trailing newlines based on the original file content. This ensures that files without a final newline are not incorrectly modified, and files with a newline retain it. - Skips processing for in-progress messages from smart apply custom models. - Simplifies the response transformation logic by consolidating newline handling and extraction into dedicated functions. - Improves test coverage to validate the newline preservation logic.
…t newline test This commit addresses two issues related to the smart apply feature: - Fixes an issue where in-progress messages were not being skipped correctly when using insert mode with smart apply custom models. The condition to skip processing for in-progress messages is updated to include the insert mode. - Removes a redundant test case in `response-transformer.test.ts` that was asserting the addition of a newline character for smart apply without an empty selection range. This test is no longer necessary due to changes in how newlines are handled.
…imit validation This commit improves the logic for selecting the entire file in the `CustomModelSelectionProvider` and adds validation to prevent exceeding the context window's input token limit. - Modifies the `getSelectedText` method to prioritize `shouldAlwaysUseEntireFile` and refactors the token limit check to occur before the full file rewrite check. - Adds a test case to verify that an error is thrown when the token count exceeds the context window input limit. - Mocks the `vscode.Range` to avoid errors during testing.
…ts for custom models This commit introduces several enhancements to the smart apply custom model feature, focusing on token handling and test coverage. - Adds tests to validate the behavior of `CustomModelSelectionProvider` under different token count scenarios, including exceeding the context window input limit, staying within limits but below a threshold, and always returning `ENTIRE_FILE` when `shouldAlwaysUseEntireFile` is true. - Updates the `trimLLMNewlines` function to handle both `\n` and `\r\n` newline characters, ensuring correct newline preservation across different operating systems. - Introduces a `SMART_APPLY_MODEL_SET` for efficient checking of smart apply custom models.
|
bf1b2c8
to
27c22b9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pr makes a couple of changes to the smart apply behavior of the new custom model such as:
CustomModelSelectionProvider
. If the token count exceeds the limit, an error is thrown.trimLLMNewlines
function to preserve or remove newlines based on the original text, ensuring consistent formatting in transformed responses.I think that if the original file didn't have a starting and ending newline our smart apply task shouldn't insert them either,
as we should stay as close as possible to what you see is what you get.
responseTransformer
to skip processing for in-progress messages andinsert
mode tasks.If we just insert the text we don't need it to be cleaned up, it's basically a copy paste job
Test plan