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

Define a new framework for chat commands #161

Merged
merged 16 commits into from
Feb 17, 2025
Merged

Conversation

dlqqq
Copy link
Member

@dlqqq dlqqq commented Feb 11, 2025

Description

Extended description

This PR introduces a modular and extensible framework for defining chat commands. To do so, this PR implements a ChatCommandRegistry singleton, which is provided to other lab extensions. Extensions can request this to add one or more chat command providers to the registry. Providers implement the IChatCommandProvider interface. Once added, each provider provides a set of commands to the command menu, and defines how those commands are handled.

This PR only aims to provide a minimal implementation of this design for further iteration. There are several known shortcomings with this PR currently, which I've documented as follow-up items after the demo section.

Demo

In this PR, 2 command providers are defined for demo purposes: one for slash commands, and one for emojis.

Screen.Recording.2025-02-10.at.4.05.15.PM.mov

The demo shows how this PR implements new features previously not possible in Jupyter AI or Jupyter Chat:

  • Commands that may come from multiple extensions are all shown together in the commands menu.
  • Command suggestions are shown for both, even though they have different patterns / syntax.
  • When an emoji command is run, it replaces the current word with the emoji.

Follow-up items

Roughly ordered in descending priority, i.e. most important first, least important last.

@dlqqq dlqqq added the enhancement New feature or request label Feb 11, 2025
@dlqqq dlqqq self-assigned this Feb 11, 2025
Copy link
Contributor

Binder 👈 Launch a Binder on branch dlqqq/jupyter-chat/chat-commands

@dlqqq dlqqq changed the title [WIP] Implement chat commands Define a new framework for chat commands Feb 14, 2025
Copy link
Collaborator

@brichet brichet left a comment

Choose a reason for hiding this comment

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

Thanks @dlqqq for working on it.

I left some comments below.

@brichet
Copy link
Collaborator

brichet commented Feb 14, 2025

In a more general test, I saw 2 issues in usages, that can be seen in the following screencast:

  • after replacing an input text using a commands, a new line replace again the text with undefined
  • the completion menu does not open if the word is not at the beginning of the input. Strangely, when printing the autocompleteProps, the open props is set to true, and the options array is not empty.
record-2025-02-14_14.00.16.webm

@dlqqq
Copy link
Member Author

dlqqq commented Feb 14, 2025

@brichet @jtpio Thank you both for the review! I've addressed your feedback and fixed the bugs that @brichet noticed.

I'll close the remaining threads once I finish opening issues. I have something really urgent to attend to at work right now, so I'll finish that on Monday hopefully.

@dlqqq
Copy link
Member Author

dlqqq commented Feb 14, 2025

Demo of latest branch state.

Screen.Recording.2025-02-14.at.2.12.21.PM.mov

Copy link
Collaborator

@brichet brichet left a comment

Choose a reason for hiding this comment

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

Thanks @dlqqq for updating the PR, I can't reproduce the issues anymore 👍 .

@dlqqq
Copy link
Member Author

dlqqq commented Feb 17, 2025

@brichet I've removed the old files. 👍

@brichet
Copy link
Collaborator

brichet commented Feb 17, 2025

Thanks @dlqqq.
The failing test are related to the old autocompletion framework.
The best option would be to update them.
Otherwise we should at least skip them, and maybe add a fix to #168 (or another issue).

@dlqqq
Copy link
Member Author

dlqqq commented Feb 17, 2025

@brichet Since CI is already failing on main, can we go ahead and merge this while tracking the CI failure in a separate issue? Merging this now would make it a lot easier to work on #171.

@dlqqq
Copy link
Member Author

dlqqq commented Feb 17, 2025

Finally finished opening all of the follow-up issues raised by other reviewers and me! Thank you all for taking the time to review this & ask questions. 🤗

@jtpio
Copy link
Member

jtpio commented Feb 17, 2025

Since CI is already failing on main, can we go ahead and merge this while tracking the CI failure in a separate issue?

Not sure it's the same failure. Based on @brichet's comment above (#161 (comment)) it seems related to the previous autocompletion registry implementation:

image

Ideally we should be able to update these tests? If it's too much effort for now, maybe worth disabling / removing them for now, but track adding them back in an issue?

@dlqqq
Copy link
Member Author

dlqqq commented Feb 17, 2025

@jtpio Thanks for the reminder! I've opened a new issue: #179

@jtpio
Copy link
Member

jtpio commented Feb 17, 2025

Nice thank!

So let's remove or disable the outdated test code in this PR?

After #148 the CI should normally be passing more consistently again (with skipped tests). And to lower maintenance burden it's nice to have a green check even though we know some tests have been disabled (it's easier to merge other PRs).

@dlqqq
Copy link
Member Author

dlqqq commented Feb 17, 2025

@jtpio Good idea, I'll do that now.

@dlqqq
Copy link
Member Author

dlqqq commented Feb 17, 2025

@jtpio Done! 👍

Copy link
Collaborator

@brichet brichet left a comment

Choose a reason for hiding this comment

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

Thanks @dlqqq and @jtpio

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.

Design proposal: Chat Completions API (rev. 1.1)
3 participants