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

Pin langchain-core dependency to prevent Settings UI crash #558

Merged
merged 2 commits into from
Jan 3, 2024

Conversation

dlqqq
Copy link
Collaborator

@dlqqq dlqqq commented Jan 3, 2024

Description

This PR is to be merged & released ASAP, as the latest releases are currently broken.

Context

LangChain now defines name as an instance field on the BaseLLM class we inherit from. This causes an unfortunate interaction with our name class attribute, where it causes Class.name to somehow always resolve to ... instead of the one stated in our class definition, which in turn is causing #555.

Even though the PR was only merged roughly a week ago, this affects Jupyter AI users because the change was made to the langchain-core package, and was released in langchain-core==0.1.4. langchain==0.0.350 (the current version of LangChain we use) used a loose version specifier that allowed this problematic version to be installed, despite our existing version lock on langchain.

You can reproduce this bug with Pydantic in Python like so:

from pydantic.v1 import BaseModel
from typing import ClassVar, Optional

class A(BaseModel):
    x: ClassVar[str] = "asdf"

class B(BaseModel):
    x: Optional[str] = None

class C(B):
    x: ClassVar[str] = "asdf"

assert A.x == "asdf"
assert C.x == "asdf" # <= this throws an exception

Root cause

After doing a bit of digging, I found the root cause of this bug in Pydantic's ModelMetaclass definition. The metaclass excludes any keys from showing up in cls.__dict__ if it's listed in cls.__dict__['__fields__'] (which only contains instance fields):

exclude_from_namespace = fields | private_attributes.keys() | {'__slots__'}
new_namespace = {
    ...
    **{n: v for n, v in namespace.items() if n not in exclude_from_namespace},
}

Tentatively, our next step is to rename this field in the LangChain source, as it seems to only be used for testing anyways. Then once that's released, we can revert this PR and bump our langchain version.

@dlqqq dlqqq added the bug Something isn't working label Jan 3, 2024
Copy link
Collaborator

@JasonWeill JasonWeill left a comment

Choose a reason for hiding this comment

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

Great work on this!

@dlqqq dlqqq enabled auto-merge (squash) January 3, 2024 01:09
@dlqqq dlqqq merged commit 550a03a into jupyterlab:main Jan 3, 2024
8 checks passed
@dlqqq
Copy link
Collaborator Author

dlqqq commented Jan 3, 2024

@meeseeksdev please backport to 1.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyter-ai that referenced this pull request Jan 3, 2024
dlqqq added a commit that referenced this pull request Jan 3, 2024
… UI crash (#559)

Co-authored-by: david qiu <david@qiu.dev>
@dlqqq dlqqq mentioned this pull request Jan 10, 2024
dbelgrod pushed a commit to dbelgrod/jupyter-ai that referenced this pull request Jun 10, 2024
…lab#558)

* pin langchain-core dependency

* ignore cookiecutter in pytest
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.

Error opening Settings UI
2 participants