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

Collaborative chat using jupyterlab_chat package #237

Closed
wants to merge 16 commits into from

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Feb 20, 2024

This PR adds a collaborative chat panel using the jupyter-chat package.

It supersedes #155

Description

  • the chat relies on CRDT and on ydoc. A chat is a a document shared between collaborators.
  • new chats can be created and user can join or leave an existing chat.
  • opening a chat file from the filebrowser opens the chat in the main area.

Peek 2024-03-25 10-44

Follow up changes

  • update chat list when a collaborator creates a chat, and notify all users (probably depends on Specify Y-WebSocket endpoint path #215)
  • notifications on incoming message
  • edition and deletion of messages

Testing this PR:

As jupyter-chat is not published on PyPI or NPMjs, this PR can only be tested locally.

Python dependencies:

Clone locally jupyter-chat then pip install .

Javascript dependencies (using yalc)

  • npm install -g yalc
  • From the jupyter-chat root directory: yalc publish
  • From the jupyter-collaboration root directory:
    • yalc add @jupyter/chat : this will create a dependencies entry in ./package.json, that we don't want for a monorepo
    • rename dependencies to resolutions in ./package.json
    • pip install -e . (or jlpm build if already installed)

EDIT
The chat package has been restarted from scratch (to restore history on chat UI files), and renamed jupyter-chat (previously jupyterlab-chat).
The commands above has been updated accordingly, please start again if you already installed jupyterlab-chat, the repo jupyterlab-chat should be removed soon.

Copy link
Contributor

Binder 👈 Launch a Binder on branch brichet/jupyter_collaboration/jupyterlab-chat

@davidbrochart
Copy link
Collaborator

Following the install instructions, I get this error:

$ jupyter lab
[W 2024-02-20 14:58:49.963 ServerApp] jupyter_collaboration | error adding extension (enabled: True): The module 'jupyter_collaboration' could not be found (No module named 'langchain'). Are you sure the extension is installed?
    Traceback (most recent call last):
      File "/home/david/github/davidbrochart/jupyterlab-chat/.pixi/envs/default/lib/python3.12/site-packages/jupyter_server/extension/manager.py", line 322, in add_extension
        extpkg = ExtensionPackage(name=extension_name, enabled=enabled)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/david/github/davidbrochart/jupyterlab-chat/.pixi/envs/default/lib/python3.12/site-packages/jupyter_server/extension/manager.py", line 186, in __init__
        self._load_metadata()
      File "/home/david/github/davidbrochart/jupyterlab-chat/.pixi/envs/default/lib/python3.12/site-packages/jupyter_server/extension/manager.py", line 201, in _load_metadata
        raise ExtensionModuleNotFound(msg) from None
    jupyter_server.extension.utils.ExtensionModuleNotFound: The module 'jupyter_collaboration' could not be found (No module named 'langchain'). Are you sure the extension is installed?

@brichet
Copy link
Contributor Author

brichet commented Feb 20, 2024

Following the install instructions, I get this error:

Thanks for testing, langchain was missing in jupyterlab_chat dependencies, I updated it.

@davidbrochart
Copy link
Collaborator

Thanks @brichet. I updated jupyterlab_chat and now I get:

[W 2024-02-20 16:17:37.005 ServerApp] 404 GET /api/chat?token=[secret] ([email protected]) 1.17ms referer=None

@brichet
Copy link
Contributor Author

brichet commented Feb 20, 2024

Thanks @brichet. I updated jupyterlab_chat and now I get:

[W 2024-02-20 16:17:37.005 ServerApp] 404 GET /api/chat?token=[secret] ([email protected]) 1.17ms referer=None

Probably the handler is not running server side.
Can you see Collaborative Chat Handlers initialized message in the console with --debug ?

@davidbrochart
Copy link
Collaborator

I reinstalled everything and now it works fine, thanks!

@ellisonbg
Copy link

@Zsailer @dlqqq for review

@brichet brichet added the enhancement New feature or request label Mar 19, 2024
@Zsailer
Copy link
Member

Zsailer commented Mar 20, 2024

As jupyter-chat is not published on PyPI or NPMjs, this PR can only be tested locally.

What's the status here? Anything blocking y'all from publishing?

@Zsailer
Copy link
Member

Zsailer commented Mar 20, 2024

I'm happy to review here whenever folks are ready. I'd love to see this land soon!

@Zsailer Zsailer mentioned this pull request Mar 20, 2024
@brichet
Copy link
Contributor Author

brichet commented Mar 21, 2024

What's the status here? Anything blocking y'all from publishing?

The package was really new and untested, we wanted to make sure it at least worked with the current implementation before publishing.
We should be able to move forward now.

@ellisonbg
Copy link

Let us know when you are ready for review.

@brichet
Copy link
Contributor Author

brichet commented Apr 1, 2024

As jupyter-chat is not published on PyPI or NPMjs, this PR can only be tested locally.

What's the status here? Anything blocking y'all from publishing?

A temporary chat-jupyter package has been published at https://www.npmjs.com/package/chat-jupyter.
It is temporary because we'd like the package to join the @jupyter namespace and the sources to be hosted under jupyter or jupyterlab organization.
Currently the sources are hosted under jupyterlab-contrib https://github.com/jupyterlab-contrib/jupyter-chat.

@brichet
Copy link
Contributor Author

brichet commented Apr 1, 2024

This PR currently includes the YChat document model, back end and front end.
The YChat is registered as a YDoc using the entry point of jupyter_ydoc. This creates a circular import, which causes pytests to fail (I don't know why only the tests fail).

The correct way would be to describe the YChat in jupyter_ydoc package. But the jupyter_ydoc used in jupyter_collaboration seems to be the one bundled with jupyterlab (and webpack).
IIUC, to be able to use this PR, this would require a release of jupyter_ydoc including YChat, and then a release of jupyterlab.

A workaround is to register the YChat using jupyter_ydoc.ydocs["jupyter_collaboration.chat_ydoc:YChat"] = YChat instead of using the entry point, to avoid circular dependency. But this is not using the public API of jupyter_ydoc.

To move forward with this PR I suggest to use the workaround and to include the YChat in jupyter_ydoc at the same time.
When the YChat will be released and usable, then we'll remove the workaround.

Any thoughts ?

@davidbrochart
Copy link
Collaborator

The YChat is registered as a YDoc using the entry point of jupyter_ydoc. This creates a circular import, which causes pytests to fail (I don't know why only the tests fail).

The circular import is expected since jupyter-collaboration depends on jupyter_ydoc, so jupyter-collaboration cannot depend on jupyter_ydoc.

The correct way would be to describe the YChat in jupyter_ydoc package. But the jupyter_ydoc used in jupyter_collaboration seems to be the one bundled with jupyterlab (and webpack).
IIUC, to be able to use this PR, this would require a release of jupyter_ydoc including YChat, and then a release of jupyterlab.

Yes, this will make it complicated.

A workaround is to register the YChat using jupyter_ydoc.ydocs["jupyter_collaboration.chat_ydoc:YChat"] = YChat instead of using the entry point, to avoid circular dependency. But this is not using the public API of jupyter_ydoc.

The problem with something like this is that, unlike using the entry points, YChat won't be registered at install time but at run-time, so it will work or not depending on the import orders. This is very fragile.

To move forward with this PR I suggest to use the workaround and to include the YChat in jupyter_ydoc at the same time.
When the YChat will be released and usable, then we'll remove the workaround.

Another way would be to have a Python package in jupyter-chat, that will properly register the YChat in the entry point. I think it makes sense to create a Python package anyway, until the API stabilizes.

@brichet
Copy link
Contributor Author

brichet commented Apr 2, 2024

The problem with something like this is that, unlike using the entry points, YChat won't be registered at install time but at run-time, so it will work or not depending on the import orders. This is very fragile.

I don't understand this point, but I'm probably missing something about the import workflow. I push my idea in 6a01b2a, but it can be reverted of course.
Since jupyter_collaboration needs to import jupyter_ydoc to register the YChat, I don't understand where the import order can be an issue.

Another way would be to have a Python package in jupyter-chat, that will properly register the YChat in the entry point. I think it makes sense to create a Python package anyway, until the API stabilizes.

Thanks, indeed this is another workaround. I'd like to avoid it because I think that jupyter-chat is supposed to be only a front end package, and should not depend on any back end technology. But, of course, if it is the more stable solution, it must be considered.

from pycrdt import Array, Map


class YChat(YBaseDoc):
class YChat(jupyter_ydoc.YBaseDoc):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand how that can work, since jupyter_ydoc doesn't export YBaseDoc:

import jupyter_ydoc

jupyter_ydoc.YBaseDoc
# Traceback (most recent call last):
#   File "<stdin>", line 1, in <module>
# AttributeError: module 'jupyter_ydoc' has no attribute 'YBaseDoc'. Did you mean: 'ybasedoc'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me neither, but i fixed it anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the YBaseDoc should be prefixed with an underscore, to specified that it should not be imported.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But it's fine to import it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, I misunderstood your comment and thought you meant not to import it.

@brichet
Copy link
Contributor Author

brichet commented Apr 2, 2024

Failures are reported in #252

@brichet
Copy link
Contributor Author

brichet commented Apr 2, 2024

After discussion with @SylvainCorlay and @davidbrochart, we wonder where this code should be located and how it should be divided. Let me know if this summary is wrong.

In each case, we thing the jupyter-chat package, providing the front end components, should be in a project in jupyterlab org. Independent of the technology used for communication, it would allow other extensions to include a chat widget.

    • the YChat model in jupyter_ydoc
    • the model/widget factories and the collaborative chat panel in jupyter_collaboration
  1. a jupyter-chat-rtc extension (or other name) that provides the YChat model, the factories and the side panel (this PR would be closed in favor of a new extension)

    • the YChat model in jupyter_ydoc
    • the model/widget factories in jupyter_collaboration
    • a jupyter-chat-rtc extension (or other name) that provides the side panel

@Zsailer @ellisonbg do you have an opinion on it ?

@brichet
Copy link
Contributor Author

brichet commented Apr 8, 2024

We decide to move the collaborative chat to a dedicated extension https://github.com/jupyterlab-contrib/jupyter-chat/tree/main/packages/jupyterlab-collaborative-chat, to have all in one repository and avoid the circular dependency mentioned here

Note that this extension already have a working shared document that allow to open a chat in the main area by opening a .chat file from the file browser.
A PR is opened to add a chat panel to manage the opened chats jupyterlab/jupyter-chat#11

@brichet brichet closed this Apr 8, 2024
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.

4 participants