Skip to content

Add GitHub Copilot integration #1311

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hideyukikodama
Copy link

Summary

This pull request adds native GitHub Copilot integration to jupyter-ai. More background and implementation details could be found in #1310.

Demo

Here is a screen capture video demonstrating how it works: Movie link


This is an initial proof of concept, and I'm very open to your feedback, improvements, or suggestions on how to better align this with the goals!

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.

@hideyukikodama Thank you for this contribution, and welcome to Jupyter AI! GitHub Copilot support has been highly requested by some users. It is great to see others contributing to this effort. 🤗

I've left some feedback below. The code looks very clean! ❤️

The only greater concern I have is with the /github slash command. We generally avoid adding provider-specific features in Jupyter AI. This helps keep Jupyter AI maintainable & vendor-neutral. Can you help document the authentication flow in this PR? In a future review, we can use that design to explore ways that this PR can support authentication without adding a /github command.

If adding a /github command turns out to be the only way to do this, we may provide this through a separate jupyter-ai-copilot package. We are open to the idea of adding model-specific packages, as long as they aren't required by jupyter-ai itself.

Comment on lines +124 to +133
class GitHubCopilotProvider(BaseProvider):
id = "github-copilot"
name = "GitHub Copilot"
models = ["*"]
model_id_key = "model"
pypi_package_deps = ["pylspclient"]
help = (
"Make sure you've installed copilot-language-server [https://www.npmjs.com/package/@github/copilot-language-server](https://www.npmjs.com/package/@github/copilot-language-server) . "
"Set this absolute path to `lsp_bin_path`."
)
Copy link
Member

Choose a reason for hiding this comment

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

Setting the list of model IDs to ["*"] has a special meaning in Jupyter AI. This denotes a registry provider, i.e. a provider where the user has to type out the model ID. Since there's only one model served by this provider, it would be better to make this explicit:

Suggested change
class GitHubCopilotProvider(BaseProvider):
id = "github-copilot"
name = "GitHub Copilot"
models = ["*"]
model_id_key = "model"
pypi_package_deps = ["pylspclient"]
help = (
"Make sure you've installed copilot-language-server [https://www.npmjs.com/package/@github/copilot-language-server](https://www.npmjs.com/package/@github/copilot-language-server) . "
"Set this absolute path to `lsp_bin_path`."
)
class GitHubCopilotProvider(BaseProvider):
id = "github"
name = "GitHub"
models = ["Copilot"]
model_id_key = "model"
pypi_package_deps = ["pylspclient"]
help = (
"Make sure you've installed copilot-language-server [https://www.npmjs.com/package/@github/copilot-language-server](https://www.npmjs.com/package/@github/copilot-language-server) . "
"Set this absolute path to `lsp_bin_path`."
)

Comment on lines +140 to +145
def __init__(
self,
**kwargs,
):
super().__init__(**kwargs)
self._llm = GitHubCopilotLLM(lsp_bin_path=self.lsp_bin_path)
Copy link
Member

Choose a reason for hiding this comment

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

The BaseProvider class allows developers to override the default definition of these 2 methods, which list the available chat & completion models:

    @classmethod
    def chat_models(self):
        """Models which are suitable for chat."""
        return self.models

    @classmethod
    def completion_models(self):
        """Models which are suitable for completions."""
        return self.models

I recommend overriding these methods to have the github:copilot model only show in the list of completion models, and be hidden from the list of chat models:

Suggested change
def __init__(
self,
**kwargs,
):
super().__init__(**kwargs)
self._llm = GitHubCopilotLLM(lsp_bin_path=self.lsp_bin_path)
def __init__(
self,
**kwargs,
):
super().__init__(**kwargs)
self._llm = GitHubCopilotLLM(lsp_bin_path=self.lsp_bin_path)
@classmethod
def chat_models(self):
"""Models which are suitable for chat."""
return []
@classmethod
def completion_models(self):
"""Models which are suitable for completions."""
return ["copilot"]

Comment on lines +50 to +55
def _signin(self):
self.ensure_lsp_server_initialized()
res = self.lsp_endpoint.call_method(
method_name="signIn",
)
return res
Copy link
Member

Choose a reason for hiding this comment

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

How does the user sign in if this method doesn't provide any environment variables / passwords? Does the Copilot LSP server open a new browser for a user to log in automatically?

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.

2 participants