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.

Sign up for GitHub

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

Jump to bottom

Refactor Chat Handlers to Simplify Initialization #1257

Merged
merged 9 commits into from
Feb 26, 2025
Merged

Refactor Chat Handlers to Simplify Initialization #1257

merged 9 commits into from
Feb 26, 2025

Conversation

Darshan808
Copy link
Member

Fixes #1256

Description

Refactored BaseChatHandler to include logs_dir and retriever as required arguments, simplifying the initialization of /ask, /generate, and /learn handlers. This removes the need for custom arguments in AskChatHandler and GenerateChatHandler.

LearnChatHandler now automatically binds itself to self.retriever, allowing /ask to perform RAG without extra logic.

Looking for feedback on any potential edge cases this might introduce!

@Darshan808 Darshan808 requested a review from dlqqq February 22, 2025 06:17
@Darshan808 Darshan808 added the enhancement New feature or request label Feb 22, 2025
@Darshan808
Copy link
Member Author

@dlqqq
As discussed in this comment #1115 (comment), should we also address the issue of moving entry points logic from extension.py to ConfigManager in this PR? If so, I'd be happy to do this with a little guidance.

@dlqqq
Copy link
Member

dlqqq commented Feb 24, 2025

@Darshan808 Thanks for working on this! I left a comment on the thread above.

should we also address the issue of moving entry points logic from extension.py to ConfigManager in this PR?

Really glad that you're thinking about this! 🤗 I recommend that this be done in a separate issue with a separate PR. Maintainers should keep their PRs focused and narrow in scope to prevent large code changes from being made all-at-once before others have a chance to review.

If you'd like to work on moving the entry points logic to the ConfigManager, it would be best to create a new issue for that. I also recommend including a basic design to describe the API changes to ConfigManager (e.g. what new properties / methods will be added?).

@Darshan808 Darshan808 marked this pull request as ready for review February 24, 2025 17:52
@Darshan808
Copy link
Member Author

@dlqqq
Could you please give it a final review. And if everything here is okay, I believe we can safely merge this and continue our work on #1249

dlqqq
dlqqq requested changes Feb 25, 2025
View reviewed changes
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.

@Darshan808 Thanks! Great work on this PR, just a couple more issues to address.

@Darshan808 Darshan808 requested a review from dlqqq February 26, 2025 03:28
dlqqq
dlqqq approved these changes Feb 26, 2025
View reviewed changes
Copy link
Member

@dlqqq dlqqq left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@Darshan808 Perfect, thank you! ❤️

This will likely need a manual backport to the 2.x branch. Can you help with that?

@dlqqq dlqqq merged commit 055d7b8 into jupyterlab:main Feb 26, 2025
9 checks passed
@dlqqq
Copy link
Member

dlqqq commented Feb 26, 2025

@meeseeksdev please backport to 2.x

@lumberbot-app Lumberbot (App)
Copy link

lumberbot-app bot commented Feb 26, 2025

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 2.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 055d7b8534eaaaa9b036fec806162da1c571f089
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #1257: Refactor Chat Handlers to Simplify Initialization'
  1. Push to a named branch:
git push YOURFORK 2.x:auto-backport-of-pr-1257-on-2.x
  1. Create a PR against branch 2.x, I would have named this PR:

"Backport PR #1257 on branch 2.x (Refactor Chat Handlers to Simplify Initialization)"

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.

Darshan808 added a commit to Darshan808/jupyter-ai that referenced this pull request Feb 27, 2025
@Darshan808
Refactor Chat Handlers to Simplify Initialization (jupyterlab#1257)
d2e8b1d 
* simplify-entrypoints-loading

* fix-lint

* fix-tests

* add-retriever-typing

* remove-retriever-from-base

* fix-circular-import(ydoc-import)

* fix-tests

* fix-type-check-failure

* refactor-retriever-init

(cherry picked from commit 055d7b8)
@Darshan808 Darshan808 mentioned this pull request Feb 27, 2025
Merged
@Darshan808
Copy link
Member Author

@dlqqq
A failing test in #1266 reveals a bug in this section. I’ve described the issue in #1266 (comment).
I think the CI passed here only because the test is marked as skipped:

@pytest.mark.skip("TODO v3: replace this with a unit test for YChatHistory")

What do you suggest ? Should we open a separate PR to address this issue ?

@dlqqq dlqqq mentioned this pull request Feb 27, 2025
Closed
dlqqq pushed a commit that referenced this pull request Feb 27, 2025
@Darshan808
Backport PR #1257 on branch 2.x (Refactor Chat Handlers to Simplify I…
a6eb979 
…nitialization) (#1266)

* Refactor Chat Handlers to Simplify Initialization (#1257)

* simplify-entrypoints-loading

* fix-lint

* fix-tests

* add-retriever-typing

* remove-retriever-from-base

* fix-circular-import(ydoc-import)

* fix-tests

* fix-type-check-failure

* refactor-retriever-init

(cherry picked from commit 055d7b8)

* fix-test-backporting

* fix-ydoc-unwanted-import

* fix-test

* remove-exception

* reorder-chat-handlers-storage
@Darshan808 Darshan808 mentioned this pull request Feb 27, 2025
Merged
srdas pushed a commit to srdas/jupyter-ai that referenced this pull request Feb 27, 2025
@Darshan808 @srdas
Refactor Chat Handlers to Simplify Initialization (jupyterlab#1257)
713701f 
* simplify-entrypoints-loading

* fix-lint

* fix-tests

* add-retriever-typing

* remove-retriever-from-base

* fix-circular-import(ydoc-import)

* fix-tests

* fix-type-check-failure

* refactor-retriever-init
@srdas
Copy link
Collaborator

srdas commented Mar 2, 2025

@Darshan808 @dlqqq As I was working on PR #1264 (unrelated to this one and not merged), I had to rebase/merge my branch to include the changes made here in this PR. After I did that, I noticed that the chat panel is not returning a response any more and just hangs, see:
image

The console log shows this error:
image

I cleaned out my entire conda environment, reinstalled it, and installed the developer version of Jupyter AI so that it includes the unreleased but merged PRs. I have used the main branch which does not contain changes from my PR, and I still get the error. @Darshan808 - can you check if the error I am getting is also evident in your installation? Thanks for the help.

@Darshan808
Copy link
Member Author

Hi @srdas, my apologies for the oversight. I just checked this branch and can confirm that the issue persists. It is caused by the problem described in #1266 (comment). which has already been addressed in #1268.

@dlqqq, could you please review #1268 ? I believe it would be best to merge it as soon as possible to resolve this. Thanks!

@srdas
Copy link
Collaborator

srdas commented Mar 2, 2025

@Darshan808 Thanks for responding so quickly and noting this -- I'll test it after PR #1268 is merged.

@Darshan808
Copy link
Member Author

@srdas Just letting you know that PR #1268 has been merged.

@srdas
Copy link
Collaborator

srdas commented Mar 3, 2025 •
edited
Loading

Thanks @Darshan808 - I tested it for v3 and v2 and it is now working after the merge.

@Darshan808
Copy link
Member Author

Is it logging the same error message ? It worked for me when I tested the branch. Could you share any helpful error message. I'll definitely look into it.

@srdas
Copy link
Collaborator

srdas commented Mar 3, 2025

Is it logging the same error message ? It worked for me when I tested the branch. Could you share any helpful error message. I'll definitely look into it.

Sorry, it is working, my error, I typed "not" instead of "now" in my message. Apologies for the confusion.

@dlqqq dlqqq mentioned this pull request Mar 3, 2025
Merged
5 tasks
Darshan808 added a commit to Darshan808/jupyter-ai that referenced this pull request Mar 4, 2025
@Darshan808
Refactor Chat Handlers to Simplify Initialization (jupyterlab#1257)
db8a53c 
* simplify-entrypoints-loading

* fix-lint

* fix-tests

* add-retriever-typing

* remove-retriever-from-base

* fix-circular-import(ydoc-import)

* fix-tests

* fix-type-check-failure

* refactor-retriever-init
dlqqq pushed a commit that referenced this pull request Mar 10, 2025
@Darshan808 @pre-commit-ci
Make Native Chat Handlers Overridable via Entry Points (#1249)
9e5eb87 
* make native chat handlers customizable

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove-ci-error

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add-disabled-check-and-sort-entrypoints

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Refactor Chat Handlers to Simplify Initialization (#1257)

* simplify-entrypoints-loading

* fix-lint

* fix-tests

* add-retriever-typing

* remove-retriever-from-base

* fix-circular-import(ydoc-import)

* fix-tests

* fix-type-check-failure

* refactor-retriever-init

* Allow chat handlers to be initialized in any order (#1268)

* lazy-initialize-retriever

* add-retriever-property

* rebase-into-main

* update-docs

* update-documentation

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
srdas pushed a commit to srdas/jupyter-ai that referenced this pull request Mar 11, 2025
@Darshan808 @pre-commit-ci @srdas
Make Native Chat Handlers Overridable via Entry Points (jupyterlab#1249)
0853b85 
* make native chat handlers customizable

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove-ci-error

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add-disabled-check-and-sort-entrypoints

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Refactor Chat Handlers to Simplify Initialization (jupyterlab#1257)

* simplify-entrypoints-loading

* fix-lint

* fix-tests

* add-retriever-typing

* remove-retriever-from-base

* fix-circular-import(ydoc-import)

* fix-tests

* fix-type-check-failure

* refactor-retriever-init

* Allow chat handlers to be initialized in any order (jupyterlab#1268)

* lazy-initialize-retriever

* add-retriever-property

* rebase-into-main

* update-docs

* update-documentation

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
srdas added a commit that referenced this pull request Mar 20, 2025
@srdas @pre-commit-ci @Darshan808
Allow embedding model fields, fix coupled model fields, add custom Op…
d4dcef9 
…enAI provider (#1264)

* Simplifying the OpenAI provider to use multiple model providers

* Update openrouter.md

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* openai general interface added

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* embedding

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Updated settings to take OpenAI generic embedding models

* added openai generic embeddings screenshot

* Fixed Issue 1261

* bump version floor on jupyter server

* linter

* adding embedding model fields

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update test_config_manager

* Update pyproject.toml

* Update pyproject.toml

* Update pyproject.toml

* pyproject.toml fixes

* pyproject.toml updates

* Update pyproject.toml

* Update pyproject.toml

* Make Native Chat Handlers Overridable via Entry Points (#1249)

* make native chat handlers customizable

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove-ci-error

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add-disabled-check-and-sort-entrypoints

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Refactor Chat Handlers to Simplify Initialization (#1257)

* simplify-entrypoints-loading

* fix-lint

* fix-tests

* add-retriever-typing

* remove-retriever-from-base

* fix-circular-import(ydoc-import)

* fix-tests

* fix-type-check-failure

* refactor-retriever-init

* Allow chat handlers to be initialized in any order (#1268)

* lazy-initialize-retriever

* add-retriever-property

* rebase-into-main

* update-docs

* update-documentation

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* pyproject toml files

* pyproject toml updates

* update snapshot

* writing config file correctly

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* tsx lint

* Update use-server-info.ts

* Update pyproject.toml

* adds embedding_models attribute

* Fixed display of Base url for embeddings and completions

* removed embedding_models

* Added help fields

* Update chat-settings.tsx

* minor reversions moved to new issue

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Darshan Poudel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlqqq dlqqq dlqqq approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request Still Needs Manual Backport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unify chat handler initialization
3 participants
@Darshan808 @dlqqq @srdas