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

Move methods generating completion replies to the provider #717

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

krassowski
Copy link
Member

This PR moves the logic for generating inline completion replies away from the DefaultInlineCompletionHandler and into the two new methods in BaseProvider:

  • generate_inline_completions: takes a request (InlineCompletionRequest) and returns InlineCompletionReply
  • stream_inline_completions: takes a request and yields an initiating reply (InlineCompletionReply) with isIncomplete set to True followed by subsequent chunks (InlineCompletionStreamChunk)

This has a few advantages:

  • we will not need to swap the completion handler to use models which do not work well with langchain templates
  • providers will be able to implement custom pre- and post-processing
  • it becomes pretty easy to create an example provider demonstrating concurrent streaming - see the MyCompletionProvider example (which is being included in documentation); it is also useful for manual testing without incurring costs by usage of cloud providers
  • the handler now has much better defined responsibility of just instantiating the LLM provider, calling these two methods, and serialising their output (and handling errors).

To make that possible I had to move the completion models to jupyter-ai-magics; I believe this is ok because the Persona model already resides in there. I kept the models.py from where I am re-exporting these models - we can either keep it like that or add a deprecation warning when someone imports from the jupyter_ai.completions.models, I am fine either way.

To avoid adding more methods on BaseProvider, I instead put the static utilities for completion methods in jupyter_ai_magics/_completion.py file; it has an underscore because I want to treat them as private implementation details so that we can change them without notice.

Additionally, I fixed typing issues in BaseInlineCompletionHandler which crept up when the initial implementation of inline completions got reworked:

  • instead of overriding .write_message() with broad types incompatible with the types in the base class (mypy complaining) I now define .reply() which only accepts the two models that should be allowed (completion reply and stream chunk)
  • incorrect return types were removed from handle_request and handle_stream_request
  • config_manger was renamed to jai_config_manager because it was overriding config_manager of JupyterHandler while having an incompatible type

@krassowski
Copy link
Member Author

Hello 👋 just checking back here. It looks like all checks are passing - is there anything which needs addressing for this PR?

Copy link
Collaborator

@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 PR looks great, thank you! ❤️ Left a minor comment below.

Based on the discussion in #717, it seems that I should merge #717 in first, then you can address the merge conflicts here. Does that work best for you?

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

Based on the discussion in #717, it seems that I should merge #717 in first, then you can address the merge conflicts here. Does that work best for you?

I am not sure which order you are suggesting but either way I can rebase once one of these is merged.

@krassowski krassowski force-pushed the completion-generation-in-provider branch 2 times, most recently from 7357dec to c16bb54 Compare April 22, 2024 22:29
@dlqqq
Copy link
Collaborator

dlqqq commented Apr 22, 2024

@krassowski Thank you for the quick reply! Sorry, I meant #726, not this PR. 🤦 I'll merge #726 in first, and then approve this PR once the conflicts are addressed and CI is passing. 👍

@dlqqq
Copy link
Collaborator

dlqqq commented Apr 22, 2024

@krassowski #726 is merged.

Add an example completion provider

Adjust name of variable to reflect new implementation
@krassowski krassowski force-pushed the completion-generation-in-provider branch from c16bb54 to 4dcf885 Compare April 22, 2024 23:06
@krassowski krassowski requested a review from dlqqq April 22, 2024 23:15
@krassowski
Copy link
Member Author

Just pinging back @dlqqq as the requested changes were made and CI is all green.

Copy link
Collaborator

@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.

Awesome work, thank you! 🎉

@dlqqq dlqqq merged commit 9c8046c into jupyterlab:main Apr 23, 2024
8 checks passed
@krassowski
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline completion failed on the server side: AttributeError: 'NoneType' object has no attribute 'ainvoke'
2 participants