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

Handle LLMs lacking concurrency support in Chat UI #506

Merged
merged 4 commits into from
Dec 19, 2023
Merged

Handle LLMs lacking concurrency support in Chat UI #506

merged 4 commits into from
Dec 19, 2023

Conversation

dlqqq
Copy link
Member

@dlqqq dlqqq commented Dec 6, 2023

Fixes #481.

@krassowski Can you help verify this fix? I haven't had good success in running GPT4All locally.

@dlqqq dlqqq added bug Something isn't working needs-backport Labels PRs that should be backported to 1.x after merge labels Dec 6, 2023
@dlqqq
Copy link
Member Author

dlqqq commented Dec 8, 2023

Actually, allows_concurrency only is respected by /generate. The default chat handler currently does nothing with this, so more work is needed.

@dlqqq dlqqq marked this pull request as draft December 8, 2023 18:01
@dlqqq dlqqq changed the title Specify no concurrency in GPT4All models Handle LLMs lacking concurrency support in Chat UI Dec 18, 2023
@dlqqq dlqqq marked this pull request as ready for review December 18, 2023 22:46
@dlqqq
Copy link
Member Author

dlqqq commented Dec 18, 2023

Just rebased and pushed a commit that blocks the user from sending another request to the LLM when it does not support concurrency, i.e. the LLM cannot process more than one prompt at a time. This is determined by the allows_concurrency instance property on LLM providers.

Very brief example screenshot from local testing with mistral-7b-openorca.Q4_0:

Screenshot 2023-12-18 at 2 48 21 PM

andrii-i
andrii-i approved these changes Dec 18, 2023
View reviewed changes
Copy link
Collaborator

@andrii-i andrii-i left a comment

Choose a reason for hiding this comment

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

Good job. With this PR, sending a new message before receiving a response to previous one doesn't crash the app anymore.

andrii-i
andrii-i reviewed Dec 18, 2023
View reviewed changes
packages/jupyter-ai/jupyter_ai/chat_handlers/base.py

# check whether the configured LLM can support a request at this time.
if self.uses_llm and BaseChatHandler._requests_count > 0:
lm_provider_klass = self.config_manager.lm_provider
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should variable be named lm_provider_class?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a common convention to prefer klass over class, since class is a reserved keyword in this language.

Copy link
Collaborator

@andrii-i andrii-i Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: it makes sense to use klass in a contexts where class is reserved or when trying to differentiate an instance vs type or consistently use klass everywhere in the codebase. Use of klass in a single variable where it doesn't serve any function except of stylistic preference makes code less readable.

krassowski
krassowski reviewed Dec 19, 2023
View reviewed changes
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you for handling this!

packages/jupyter-ai/jupyter_ai/chat_handlers/base.py Outdated Show resolved Hide resolved
@JasonWeill JasonWeill merged commit 79e345a into jupyterlab:main Dec 19, 2023
7 checks passed
@dlqqq
Copy link
Member Author

dlqqq commented Dec 19, 2023

@meeseeksdev please backport to 1.x

@lumberbot-app Lumberbot (App)
Copy link

lumberbot-app bot commented Dec 19, 2023

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
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 79e345a57db9aa170727a254410783cc422a6654
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #506: Handle LLMs lacking concurrency support in Chat UI'
  1. Push to a named branch:
git push YOURFORK 1.x:auto-backport-of-pr-506-on-1.x
  1. Create a PR against branch 1.x, I would have named this PR:

"Backport PR #506 on branch 1.x (Handle LLMs lacking concurrency support in Chat UI)"

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.

@dlqqq
Copy link
Member Author

dlqqq commented Dec 19, 2023

It looks like the backport is failing because #490 was not backported. I'll do that now.

@dlqqq
Copy link
Member Author

dlqqq commented Dec 19, 2023

@meeseeksdev please backport to 1.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyter-ai that referenced this pull request Dec 19, 2023
@dlqqq @meeseeksmachine
Backport PR jupyterlab#506: Handle LLMs lacking concurrency support i…
1350b61 
…n Chat UI
@meeseeksmachine meeseeksmachine mentioned this pull request Dec 19, 2023
Merged
dlqqq added a commit that referenced this pull request Dec 19, 2023
@meeseeksmachine @dlqqq
Backport PR #506: Handle LLMs lacking concurrency support in Chat UI (#…
e76e352 
…529)

Co-authored-by: david qiu <david@qiu.dev>
dbelgrod pushed a commit to dbelgrod/jupyter-ai that referenced this pull request Jun 10, 2024
@dlqqq
Handle LLMs lacking concurrency support in Chat UI (jupyterlab#506)
b1bb833 
* specify no concurrency in GPT4All models

* handle LLMs lacking concurrency support in chat

* add annotation & default for uses_llm attr

* edit reply msg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krassowski krassowski krassowski left review comments

@andrii-i andrii-i andrii-i approved these changes

@JasonWeill JasonWeill JasonWeill left review comments

Assignees
No one assigned
Labels
bug Something isn't working needs-backport Labels PRs that should be backported to 1.x after merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Segmentation fault (core dumped) with gpt4all models during concurrent execution
4 participants
@dlqqq @krassowski @andrii-i @JasonWeill