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

DEPS: Revert SQLAlchemy minimum version back to 1.4.36 #60977

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

snitish
Copy link
Contributor

@snitish snitish commented Feb 21, 2025

@snitish snitish requested a review from mroeschke as a code owner February 21, 2025 07:41
@snitish snitish marked this pull request as draft February 21, 2025 07:41
@snitish snitish changed the title BUG: Revert SQLAlchemy minimum version back to 1.4.36 DEPS: Revert SQLAlchemy minimum version back to 1.4.36 Feb 21, 2025
@snitish
Copy link
Contributor Author

snitish commented Feb 21, 2025

@mroeschke given there've been so many requests for this, thought I'd give this a try. The minimum version for SQLAlchemy was bumped from 1.4.36 to 2.0.0 about a year ago (#55524). I'm resetting it back to 1.4.36.
My local tests ran fine with this version but is there a way to configure CI tests to run on both SQLAlchemy versions?

@swarajban
Copy link

I don't know if just changing the versions will fix the underlying issues. When we use an old version of SA, functions like pandasSQL_builder don't parse the connection object properly and return SQLite (which causes other downstream issues, eg params to not work correctly)

@snitish
Copy link
Contributor Author

snitish commented Feb 21, 2025

I don't know if just changing the versions will fix the underlying issues. When we use an old version of SA, functions like pandasSQL_builder don't parse the connection object properly and return SQLite (which causes other downstream issues, eg params to not work correctly)

The reason it was incorrectly returning an SQLite object is because of this line:

sqlalchemy = import_optional_dependency("sqlalchemy", errors="ignore")

If SQLAlchemy doesn't satisfy the minimum version, the import fails and it leads to creating an SQLite object instead. If the minimum version is updated, the code works fine. Tested it on my branch after updating SQLAlchemy to 1.4.

>>> import pandas as pd
>>> from sqlalchemy import create_engine
f = pd.DataFrame({'a': [0, 1, 2, 3]})
df.to_sql('test', con)>>> con = create_engine('sqlite:///:memory:')
>>> df = pd.DataFrame({'a': [0, 1, 2, 3]})
>>> df.to_sql('test', con)
4

@swarajban
Copy link

I don't know if just changing the versions will fix the underlying issues. When we use an old version of SA, functions like pandasSQL_builder don't parse the connection object properly and return SQLite (which causes other downstream issues, eg params to not work correctly)

The reason it was incorrectly returning an SQLite object is because of this line:

sqlalchemy = import_optional_dependency("sqlalchemy", errors="ignore")

If SQLAlchemy doesn't satisfy the minimum version, the import fails and it leads to creating an SQLite object instead. If the minimum version is updated, the code works fine. Tested it on my branch after updating SQLAlchemy to 1.4.

>>> import pandas as pd
>>> from sqlalchemy import create_engine
f = pd.DataFrame({'a': [0, 1, 2, 3]})
df.to_sql('test', con)>>> con = create_engine('sqlite:///:memory:')
>>> df = pd.DataFrame({'a': [0, 1, 2, 3]})
>>> df.to_sql('test', con)
4

Ah wow, well gl! We would really appreciate this change

@mroeschke
Copy link
Member

is there a way to configure CI tests to run on both SQLAlchemy versions?

There is a Minimum Versions build that will test with the pinned, minimum supported version of the dependencies (1.4.x), and the rest of the builds test with dependencies pinned as >=version so those builds should test with 2.0.x.

@snitish
Copy link
Contributor Author

snitish commented Feb 22, 2025

Thanks @mroeschke . Confirmed that the minimum version tests have passed. The failing Ubuntu tests are apparently due to an unrelated issue in libsqlite-3.49.1. (See AUTOMATIC1111/stable-diffusion-webui#16856)
I'm still not sure why these tests are passing for other PRs - they seem to be using the older 3.48.1 version - but my guess is that updating the minimum versions file may have triggered the build to re-fetch the latest versions of dependencies.

@snitish snitish marked this pull request as ready for review February 23, 2025 23:05
@mroeschke
Copy link
Member

Could you add a whatsnew note in v2.3.0? I think it's reasonable to backport this since many users were reporting this issue

@mroeschke
Copy link
Member

And after this PR, do you mind submitting one to our conda-feedstock to updating sqlalchemy version there? https://github.com/conda-forge/pandas-feedstock/blob/main/recipe/meta.yaml

@mroeschke mroeschke added IO SQL to_sql, read_sql, read_sql_query Dependencies Required and optional dependencies labels Feb 26, 2025
@mroeschke mroeschke added this to the 2.3 milestone Feb 26, 2025
@snitish
Copy link
Contributor Author

snitish commented Feb 26, 2025

@mroeschke should the whatsnew note go under the 'Other Enhancements' section? Or should I create a new section for minimum version updates?

@mroeschke
Copy link
Member

should the whatsnew note go under the 'Other Enhancements' section?

Yup, I think this section is appropriate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependencies Required and optional dependencies IO SQL to_sql, read_sql, read_sql_query
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Pandas 2.2 breaks SQLAlchemy 1.4 compatibility
3 participants