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

Improves the relevance of codestral completion #18

Merged
merged 5 commits into from
Dec 3, 2024

Conversation

brichet
Copy link
Collaborator

@brichet brichet commented Nov 8, 2024

This PR try to improve the relevance of code completion using codestral.

Should fix #16

Context

Codestral allows only one request per second, and a completion request may takes about 2 sec.
This can lead to inconsistency between the suggestion and the current prompt, which may have change during the request.

Code change

The completion provider keep in memory the latest prompt if the a request is pending.
When the suggestion is received, there are 3 possible situations:

  • the prompt hasn't change, the suggestion is returned as it
  • the prompt has been updated but still contains the previous one. For example the fetch may have been requested with def te and the current prompt is def test.
    • if the suggestion contains the update, the updated part is removed from the suggestion and return it
    • otherwise, a new completion is requested
  • too many differences between the invoked prompt and the current one, a new completion is requested

The completion may take more time to be display, but should be more relevant.

Additionally, the options Debouncer delay in the inline completion settings should be used, to add a delay before fetching the completion after the last key has been pressed.

EDIT: I forgot to mention that it also adds a timeout of 3s, and fetch again if there is no respond in this delay. This is to avoid some infinite waiting from server.

@brichet brichet marked this pull request as ready for review November 8, 2024 09:22
@brichet brichet added the bug Something isn't working label Nov 8, 2024
@brichet brichet requested a review from jtpio November 8, 2024 09:36
@jtpio
Copy link
Member

jtpio commented Nov 8, 2024

the prompt has been updated but still contains the previous one. For example the fetch may have been requested with def te and the current prompt is def test.

Probably this could indeed help, although when typing it's common to go back, delete a few characters, type new ones. So not sure if it really helps in practice?

Also wondering if this may be adding additional complexity to the inline completer logic to work around the slow response time from the Mistral API? Maybe it's already a bit faster with other providers?

@brichet
Copy link
Collaborator Author

brichet commented Nov 12, 2024

Probably this could indeed help, although when typing it's common to go back, delete a few characters, type new ones. So not sure if it really helps in practice?

I agree that it may not often be useful, it only avoid fetching again the completion if the previous one includes the changes...
This part is only an extra of the PR, the main goal is to avoid displaying outdated suggestions, even if it takes time.

Also wondering if this may be adding additional complexity to the inline completer logic to work around the slow response time from the Mistral API?

The complexity is mostly added to the codestral completer, it should not be ported to other completers if not required.

@brichet
Copy link
Collaborator Author

brichet commented Nov 19, 2024

I forgot to mention that it also adds a timeout of 3s, and fetch again if there is no respond in this delay. This is to avoid some infinite waiting from server.

@brichet
Copy link
Collaborator Author

brichet commented Dec 2, 2024

@jtpio should we move forward with this PR ?
If you still think that this PR adds complexity, I can remove the part comparing the diff between the current prompt and the suggestion.

The changes from this PR would still avoid displaying outdated suggestions, even if the suggestion can take a long time to be available.

@jtpio
Copy link
Member

jtpio commented Dec 2, 2024

If you still think that this PR adds complexity, I can remove the part comparing the diff between the current prompt and the suggestion.

Ah yes, maybe it can be a good tradeoff for now for getting that one in.

Also if this change is just for the Mistral completion we could indeed get this in, if it already improves the current behavior.

@brichet
Copy link
Collaborator Author

brichet commented Dec 2, 2024

Ah yes, maybe it can be a good tradeoff for now for getting that one in.

Done

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

Let's get this in as this should already improve the current situation.

@jtpio jtpio merged commit a5a3bd6 into jupyterlite:main Dec 3, 2024
7 checks passed
@brichet brichet deleted the codestral_completion branch December 3, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usability issues with the inline completer
2 participants