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

Import of declarative_base when SQLAlchemy <1.4 #883

Merged
merged 5 commits into from
Feb 11, 2023

Conversation

s16h
Copy link
Contributor

@s16h s16h commented Feb 4, 2023

In pyproject.toml, the expectation is SQLAlchemy = "^1". But, the way declarative_base is imported in cache.py will only work with SQLAlchemy >=1.4. This PR makes sure Langchain can be run in environments with SQLAlchemy <1.4

@s16h s16h force-pushed the shahriar/sqlalchemy-backcompat branch from f61ce08 to eb51dd7 Compare February 4, 2023 16:16
@s16h
Copy link
Contributor Author

s16h commented Feb 4, 2023

In general, it feels like SQLAlchemy should be made to be an optional dependency. What do you think, @hwchase17?

@hwchase17
Copy link
Contributor

In general, it feels like SQLAlchemy should be made to be an optional dependency. What do you think, @hwchase17?

yeah let me take a look into that

im also fine just bumping the requirements to be "^1.4" - thoughts on that?

@s16h
Copy link
Contributor Author

s16h commented Feb 4, 2023

im also fine just bumping the requirements to be "^1.4" - thoughts on that?

The move from 1.3 to 1.4 isn't trivial (i.e., it's not just a matter of upgrading, as I learnt earlier today). So, that'd possibly make life difficult for LangChain users — that's certainly the case for us. Given how popular <1.4 is in the wild, I think it'd make sense to support it. Just my two cents.

@s16h s16h force-pushed the shahriar/sqlalchemy-backcompat branch from 43fb8d0 to 3d5f8b0 Compare February 10, 2023 23:28
@s16h
Copy link
Contributor Author

s16h commented Feb 10, 2023

I know you're busy with exciting stuff @hwchase17 (💪), but, thoughts on this? 😊

@hwchase17
Copy link
Contributor

I know you're busy with exciting stuff @hwchase17 (💪), but, thoughts on this? 😊

sorry for delay - agree this should be fixed. will get into the next release

@hwchase17 hwchase17 merged commit b774701 into langchain-ai:master Feb 11, 2023
dongreenberg pushed a commit to dongreenberg/langchain that referenced this pull request Feb 17, 2023
In
[pyproject.toml](https://github.com/hwchase17/langchain/blob/master/pyproject.toml),
the expectation is `SQLAlchemy = "^1"`. But, the way `declarative_base`
is imported in
[cache.py](https://github.com/hwchase17/langchain/blob/master/langchain/cache.py)
will only work with SQLAlchemy >=1.4. This PR makes sure Langchain can
be run in environments with SQLAlchemy <1.4
@blob42 blob42 mentioned this pull request Feb 21, 2023
zachschillaci27 pushed a commit to zachschillaci27/langchain that referenced this pull request Mar 8, 2023
In
[pyproject.toml](https://github.com/hwchase17/langchain/blob/master/pyproject.toml),
the expectation is `SQLAlchemy = "^1"`. But, the way `declarative_base`
is imported in
[cache.py](https://github.com/hwchase17/langchain/blob/master/langchain/cache.py)
will only work with SQLAlchemy >=1.4. This PR makes sure Langchain can
be run in environments with SQLAlchemy <1.4
eyurtsev pushed a commit that referenced this pull request Oct 4, 2023
<!-- Thank you for contributing to LangChain!

Replace this entire comment with:
  - **Description:** a description of the change, 
  - **Issue:** the issue # it fixes (if applicable),
  - **Dependencies:** any dependencies required for this change,
- **Tag maintainer:** for a quicker response, tag the relevant
maintainer (see below),
- **Twitter handle:** we announce bigger features on Twitter. If your PR
gets announced, and you'd like a mention, we'll gladly shout you out!

Please make sure your PR is passing linting and testing before
submitting. Run `make format`, `make lint` and `make test` to check this
locally.

See contribution guidelines for more information on how to write/run
tests, lint, etc:

https://github.com/langchain-ai/langchain/blob/master/.github/CONTRIBUTING.md

If you're adding a new integration, please include:
1. a test for the integration, preferably unit tests that do not rely on
network access,
2. an example notebook showing its use. It lives in `docs/extras`
directory.

If no one reviews your PR within a few days, please @-mention one of
@baskaryan, @eyurtsev, @hwchase17.
 -->

## Description
Currently SQLAlchemy >=1.4.0 is a hard requirement. We are unable to run
`from langchain.vectorstores import FAISS` with SQLAlchemy <1.4.0 due to
top-level imports, even if we aren't even using parts of the library
that use SQLAlchemy. See Testing section for repro. Let's make it so
that langchain is still compatible with SQLAlchemy <1.4.0, especially if
we aren't using parts of langchain that require it.

The main conflict is that SQLAlchemy removed `declarative_base` from
`sqlalchemy.ext.declarative` in 1.4.0 and moved it to `sqlalchemy.orm`.
We can fix this by try-catching the import. This is the same fix as
applied in #883.

(I see that there seems to be some refactoring going on about isolating
dependencies, e.g.
c87e9fb,
so if this issue will be eventually fixed by isolating imports in
langchain.vectorstores that also works).

## Issue
I can't find a matching issue.

## Dependencies
No additional dependencies

## Maintainer
@hwchase17 since you reviewed
#883

## Testing
I didn't add a test, but I manually tested this.

1. Current failure:
```
langchain==0.0.305
sqlalchemy==1.3.24
```

``` python
python -i
>>> from langchain.vectorstores import FAISS
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/pay/src/zoolander/vendor3/lib/python3.8/site-packages/langchain/vectorstores/__init__.py", line 58, in <module>
    from langchain.vectorstores.pgembedding import PGEmbedding
  File "/pay/src/zoolander/vendor3/lib/python3.8/site-packages/langchain/vectorstores/pgembedding.py", line 10, in <module>
    from sqlalchemy.orm import Session, declarative_base, relationship
ImportError: cannot import name 'declarative_base' from 'sqlalchemy.orm' (/pay/src/zoolander/vendor3/lib/python3.8/site-packages/sqlalchemy/orm/__init__.py)
```

2. This fix:
```
langchain==<this PR>
sqlalchemy==1.3.24
```

``` python
python -i
>>> from langchain.vectorstores import FAISS
<succeeds>
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants