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 prefix removal when streaming inline completions #879

Merged
merged 2 commits into from
Jul 8, 2024

Conversation

krassowski
Copy link
Member

Fixes #841

@krassowski krassowski added the bug Something isn't working label Jul 7, 2024
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@krassowski This looks good aside from some minor typos which I've called out below, thank you for making this change! Do you recall the previous reason why we were only trimming the prefix conditionally when the response began/ended with ```? Want to verify that we're not making a regression unexpectedly.

packages/jupyter-ai-magics/jupyter_ai_magics/providers.py Outdated Show resolved Hide resolved
packages/jupyter-ai-magics/jupyter_ai_magics/providers.py Outdated Show resolved Hide resolved
packages/jupyter-ai-magics/jupyter_ai_magics/providers.py Outdated Show resolved Hide resolved
packages/jupyter-ai-magics/jupyter_ai_magics/providers.py Outdated Show resolved Hide resolved
@krassowski
Copy link
Member Author

krassowski commented Jul 8, 2024

Do you recall the previous reason why we were only trimming the prefix conditionally when the response began/ended with ```? Want to verify that we're not making a regression unexpectedly.

This was just a premature optimization. Briefly, the idea was that instead of storing two versions of the suggestion text (as it is now done in this PR with suggestion and processed_suggestion) we could store only one (the processed version).

So with the previous logic (where we wanted to store only one version) if we got the following reply:

```python
print("test")
```

in four chunks:

```
python

print("test")

```

then the old logic (if we did not have the conditional) would trim the first chunk away, but accept second and third chunk (and possibly the fourth - I do not remember), so the result would be:

python
print("test")

or (if the code was only trimming the closing fence if it detected an opening fence - which I think a version of the code did)

python
print("test")
```

In either case, we would not want this to happen, the intended result is just:

print("test")

Now after this PR we are reapplying post-processing after each fragment and do not need to worry about these. While this has a negligibly higher cost, it is even more appropriate because the post_process_suggestion method is now a part of the provider API so we do not have a control on what is inside for custom providers (it may not even look at ```!).

@krassowski krassowski requested a review from dlqqq July 8, 2024 20:54
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@krassowski Thanks for lending context & addressing my feedback! Proceeding to merge.

@dlqqq dlqqq force-pushed the fix-stream-prefix-removal branch from 85a3f31 to 267a8b9 Compare July 8, 2024 21:26
@dlqqq dlqqq changed the title Fix removal of prefix when streaming Fix prefix removal when streaming inline completions Jul 8, 2024
@dlqqq dlqqq enabled auto-merge (squash) July 8, 2024 21:27
@dlqqq dlqqq merged commit ab38fe7 into jupyterlab:main Jul 8, 2024
9 checks passed
Marchlak pushed a commit to Marchlak/jupyter-ai that referenced this pull request Oct 28, 2024
* Fix removal of prefix when streaming

* Fix typo

Co-authored-by: david qiu <[email protected]>

---------

Co-authored-by: david qiu <[email protected]>
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.

Prefix is not removed from inline completions when streaming
2 participants