Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

DEPS: Revert SQLAlchemy minimum version back to 1.4.36#60977

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
snitish wants to merge7 commits intopandas-dev:main
base:main
Choose a base branch
Loading
fromsnitish:issue57049

Conversation

snitish
Copy link
Member

@snitishsnitish commentedFeb 21, 2025
edited
Loading

anentropic, aldenks, and T1nG3R reacted with thumbs up emoji
@snitishsnitish marked this pull request as draftFebruary 21, 2025 07:41
@snitishsnitish changed the titleBUG: Revert SQLAlchemy minimum version back to 1.4.36DEPS: Revert SQLAlchemy minimum version back to 1.4.36Feb 21, 2025
@snitish
Copy link
MemberAuthor

@mroeschke given there've been so many requests for this, thought I'd give this a try. The minimum version for SQLAlchemy was bumped from1.4.36 to2.0.0 about a year ago (#55524). I'm resetting it back to1.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?

miraculixx reacted with thumbs up emoji

@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 likepandasSQL_builder don't parse the connection object properly and return SQLite (which causes other downstream issues, egparams to not work correctly)

@snitish
Copy link
MemberAuthor

I don't know if just changing the versions will fix the underlying issues. When we use an old version of SA, functions likepandasSQL_builder don't parse the connection object properly and return SQLite (which causes other downstream issues, egparams 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_enginef = 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 reacted with hooray emoji

@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 likepandasSQL_builder don't parse the connection object properly and return SQLite (which causes other downstream issues, egparams 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_enginef = 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

snitish reacted with thumbs up emoji

@mroeschke
Copy link
Member

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

There is aMinimum 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 reacted with thumbs up emoji

@snitish
Copy link
MemberAuthor

snitish commentedFeb 22, 2025
edited
Loading

Thanks@mroeschke . Confirmed that the minimum version testshave passed. The failing Ubuntu tests are apparently due to an unrelated issue in libsqlite-3.49.1. (SeeAUTOMATIC1111/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.

@snitishsnitish marked this pull request as ready for reviewFebruary 23, 2025 23:05
@mroeschke
Copy link
Member

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

snitish reacted with thumbs up emoji

@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

snitish reacted with thumbs up emoji

@mroeschkemroeschke added IO SQLto_sql, read_sql, read_sql_query DependenciesRequired and optional dependencies labelsFeb 26, 2025
@mroeschkemroeschke added this to the2.3 milestoneFeb 26, 2025
@snitish
Copy link
MemberAuthor

@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

@jabbera
Copy link

Is this going to be merged?!?! I cannot tell you how excited this makes me. The amount of pain this issue has caused me knows no bound.

miraculixx reacted with thumbs up emojihail100, robzor92, and artur-sniegowski reacted with eyes emoji

@github-actionsGitHub Actions
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Pleaseupdate and respond to this comment if you're still interested in working on this.

miraculixx reacted with thumbs up emoji

@miraculixx
Copy link

miraculixx commentedApr 16, 2025
edited
Loading

Much appreciate this work. How can we help to get this merged and included in a release? 🙏🏼

@simonjayhawkins
Copy link
Member

pre-commit.ci autofix

pre-commit-ci[bot] reacted with rocket emoji

@github-actionsGitHub Actions
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Pleaseupdate and respond to this comment if you're still interested in working on this.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@simonjayhawkinssimonjayhawkinssimonjayhawkins approved these changes

@mroeschkemroeschkeAwaiting requested review from mroeschkemroeschke is a code owner

Assignees
No one assigned
Labels
DependenciesRequired and optional dependenciesIO SQLto_sql, read_sql, read_sql_queryStale
Projects
None yet
Milestone
2.3
Development

Successfully merging this pull request may close these issues.

BUG: Pandas 2.2 breaks SQLAlchemy 1.4 compatibility
6 participants
@snitish@swarajban@mroeschke@jabbera@miraculixx@simonjayhawkins

[8]ページ先頭

©2009-2025 Movatter.jp