Base chat handler refactor for custom slash commands

[JasonWeill]

Prep for #351.

Refactors chat handlers to use the entry points framework.

Known issue: The /help message does not pull from chat handlers' help messages. (#436)

[dlqqq]

Also, I think from our discussion ~1 month ago, we don’t want to load our native chat handlers (i.e. the ones we define) via entry points. This is because they may be overridden by another package accidentally/maliciously defining another chat handler of the same ID.
That would mean that we can statically declare our native chat handlers like the old approach, which also conveniently avoids the learn/ask interdependency you encountered:

        jai_chat_handlers = {
            "default": default_chat_handler,
            "/ask": ask_chat_handler,
            "/clear": clear_chat_handler,
            "/generate": generate_chat_handler,
            "/learn": learn_chat_handler,
            "/help": help_chat_handler,
        }

We will still use entry points, but only to extend this dictionary, not to initialize it. Let me know what you think of this, I may be forgetting something important.

[JasonWeill]

This is still a WIP; I'm getting an error when passing a message to my custom handler /custom, defined in the pyproject.toml as:

[project.entry-points."jupyter_ai.chat_handlers"]
custom = "jupyter_ai:chat_handlers.CustomChatHandler"

Note the dot (not colon) before CustomChatHandler, per @dlqqq.

The error is:

future: <Task finished name='Task-7950' coro=<RootChatHandler._route() done, defined at /Users/jweill/git/jupyter-ai/packages/jupyter-ai/jupyter_ai/handlers.py:211> exception=TypeError("BaseChatHandler.process_message() missing 1 required positional argument: 'message'")>
Traceback (most recent call last):
  File "/Users/jweill/git/jupyter-ai/packages/jupyter-ai/jupyter_ai/handlers.py", line 225, in _route
    await self.chat_handlers[command].process_message(message)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: BaseChatHandler.process_message() missing 1 required positional argument: 'message'

[JasonWeill]

Updated per @dlqqq 's feedback. Tested all of the commands, including the /custom command, and they work locally.

[Zsailer]

Also, I think from our discussion ~1 month ago, we don’t want to load our native chat handlers (i.e. the ones we define) via entry points. This is because they may be overridden by another package accidentally/maliciously defining another chat handler of the same ID. That would mean that we can statically declare our native chat handlers like the old approach, which also conveniently avoids the learn/ask interdependency you encountered:

        jai_chat_handlers = {
            "default": default_chat_handler,
            "/ask": ask_chat_handler,
            "/clear": clear_chat_handler,
            "/generate": generate_chat_handler,
            "/learn": learn_chat_handler,
            "/help": help_chat_handler,
        }

We will still use entry points, but only to extend this dictionary, not to initialize it. Let me know what you think of this, I may be forgetting something important.

On the other hand, what if a consumer of this plugin does want to override the /ask, /clear, /generate, etc. handlers defined natively here? Is this something that should be blocked?

[Zsailer]

This is sweet @JasonWeill! I love this refactor. Defining custom commands is a major win for this plugin.

I can't tell when briefly scanning the frontend plugin... do the custom commands appear in the UI of the side panel at the top of the chat?

[JasonWeill]

This is sweet @JasonWeill! I love this refactor. Defining custom commands is a major win for this plugin.

I can't tell when briefly scanning the frontend plugin... do the custom commands appear in the UI of the side panel at the top of the chat?

@Zsailer Not yet; making the help message dynamic is covered by #436 (and making it customizable is covered separately by #279).

[JasonWeill]

On the other hand, what if a consumer of this plugin does want to override the /ask, /clear, /generate, etc. handlers defined natively here? Is this something that should be blocked?

@Zsailer Our first goal here is to allow for additional slash commands. Overriding the existing ones is currently out of scope, but we can have another issue if that's something that a user/developer wants to do.

[3coins]

@Zsailer

On the other hand, what if a consumer of this plugin does want to override the /ask, /clear, /generate, etc. handlers defined > natively here? Is this something that should be blocked?

This is a good point, and agree that there are valid use cases where this can provide value. The main hesitation for adding this was any use of these to override builtin commands to do bad or unexpected things, which would hurt trust in Jupyter AI. We are open to revisiting this, and would love to hear your thoughts on this.

[Zsailer]

override builtin commands to do bad or unexpected things, which would hurt trust in Jupyter AI.

Sure, but it is Python and a bad actor can simply replace jai_chat_handlers setting from their own package/extension... which is exactly what I would to do to replace these values as a "good actor" 😉. Unfortunately, you can't get around this hackery.

For my immediate use-case, I want to disable some of the default chat handlers (not necessarily replace them) because they don't work well on my system. This already requires me to override the jai_chat_handlers setting. In a not-too-distance future, I could see replacing handlers with my own version that works for my system.

For the "good actors" who want to replace the default handlers—if you allows them to override using the entry-point system, you can detect this and log a warning to the user that the default command is being replaced.

[JasonWeill]

@Zsailer I opened #507 to add a facility to override, or perhaps to remove, built-in commands. I think #436 should be a prerequisite for the latter; currently, the help message is static, so we would need to generate it dynamically instead. Thanks again for your response!

Changes made as requested

[dlqqq]

@meeseeksdev please backport to 1.x

[lumberbot-app]

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

git checkout 1.x
git pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 64e6daf42b34f6906749de4ec3bafeca42b77210

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #398: Base chat handler refactor for custom slash commands'

4. Push to a named branch:

git push YOURFORK 1.x:auto-backport-of-pr-398-on-1.x

5. Create a PR against branch 1.x, I would have named this PR:

"Backport PR #398 on branch 1.x (Base chat handler refactor for custom slash commands)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.