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

Use new langchain-openai partner package #653

Merged
merged 7 commits into from
Mar 27, 2024
Merged

Use new langchain-openai partner package #653

merged 7 commits into from
Mar 27, 2024

Conversation

startakovsky
Copy link
Contributor

@startakovsky startakovsky commented Feb 25, 2024

This fixes #652 , I think. I wonder if I needed to create some tests. Hope this helps get a head start on this issue. What a great tool this is.

Copy link

welcome bot commented Feb 25, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@startakovsky
Copy link
Contributor Author

@jtpio I really don't have the time to build up the Jupyter AI development environment but since I was working with it yesterday I figured I'd create a PR for a bug I'd noticed.

I would like to note that the tests passed, which means there was never a test to check this code path, which seems worth noting given how common I'd assume it is.

I hope this PR doesn't break anything and would encourage someone to actually test it.

@krassowski krassowski added the bug Something isn't working label Feb 25, 2024
@dlqqq
Copy link
Member

dlqqq commented Mar 5, 2024

@startakovsky Thank you for this PR! I was out sick last week, so I only just got the chance to review this. Right now, I'm gathering more info on this issue, and will return with feedback.

Appreciate your patience! 🤗

@dlqqq dlqqq added this to the Priority milestone Mar 7, 2024
@dlqqq dlqqq changed the title Import ChatOpenAI/AzureChatOpenAI from the updated langchain location Use new langchain-openai partner package Mar 26, 2024
@dlqqq
Copy link
Member

dlqqq commented Mar 26, 2024

@startakovsky Thank you for contributing this PR! Sorry that it had escaped my attention for a while, but I just got a chance to review this again.

I pushed some commits that:

  • Keeps the langchain_openai package optional by moving the providers to an isolated module
  • Updates the documentation to refer to langchain_openai as a dependency, remove the direct dependencies on openai and tiktoken.

@dlqqq dlqqq requested a review from 3coins March 26, 2024 22:42
@3coins
Copy link
Collaborator

3coins commented Mar 27, 2024

@dlqqq
Thanks for making the updates. Code looks good.

One side-effect of this change is that if users upgrade to this version, and not have thelangchain-openai package installed, they will see None selected as the provider.

Screenshot 2024-03-26 at 6 44 20 PM

Fixing this is beyond the scope of this PR, but we probably need a richer experience in settings or chat to capture that there is a dependent package missing.

@startakovsky
Copy link
Contributor Author

startakovsky commented Mar 27, 2024

Thanks guys, seems good to go!

One thing: Is there a need to have one or more tests on this code path?

@dlqqq
Copy link
Member

dlqqq commented Mar 27, 2024

@3coins Thank you for the review! This is expected. If the model provider cannot be loaded by the ConfigManager, the ConfigManager sets the language model to None.

I agree that this is something we should address with better messaging (e.g. a warning banner), and that this should be done in a future PR.

@startakovsky There's less utility in adding tests here, since we are mainly relying on logic from the langchain-openai partner package, which has its own suite of unit tests. 👍

@dlqqq dlqqq merged commit 4a63361 into jupyterlab:main Mar 27, 2024
8 checks passed
Copy link

welcome bot commented Mar 27, 2024

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@dlqqq
Copy link
Member

dlqqq commented Mar 27, 2024

@meeseeksdev please backport to 1.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyter-ai that referenced this pull request Mar 27, 2024
dlqqq pushed a commit that referenced this pull request Mar 27, 2024
@startakovsky startakovsky deleted the fix-gpt-4-error branch April 7, 2024 13:30
Marchlak pushed a commit to Marchlak/jupyter-ai that referenced this pull request Oct 28, 2024
* Import ChatOpenAI and AzureChatOpenAI from the updated langchain location:

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

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

* Update pyproject.toml, add `langchain-openai` as an optional dependency

* remove direct dependency on openai and tiktoken packages

* move openai providers to separate module to keep langchain_openai optional

* update openai provider dependencies in docs

* pre-commit

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: David L. Qiu <[email protected]>
@dlqqq dlqqq removed this from the Priority milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent behavior with %%ai openai-chat:gpt-4 and %%ai openai-chat:gpt-3.5-turbo
4 participants