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

SQLAlchemy 1.x and 2.x compatibility: Use explicit transactions, remove sqlalchemy version constraint #1908

Merged
merged 4 commits into from
Jan 30, 2024

Conversation

RobinL
Copy link
Member

@RobinL RobinL commented Jan 30, 2024

Closes #1785 and #1906 and #1786

It turns out that the UDF registration only works correctly in sqlalchemy 2.0 if you use begin, which starts a transaction explicitly. otherwise subsequent function registrations can't always 'see' previous ones.

fetchall() no longer works in sqlalchemy 2.0.0 see **here but luckily .mappings().all() seems to work fine

I have checked, and the following work:

sqlalchemy.__version__='1.4.49'
pd.__version__='1.5.0'

sqlalchemy.__version__='2.0.25'
pd.__version__='1.5.0'

sqlalchemy.__version__='2.0.25'
pd.__version__='2.2.0'

The only combination that doesn't is:

sqlalchemy.__version__='1.4.49'
pd.__version__='2.2.0'

AttributeError: 'Engine' object has no attribute 'cursor'

But that's fine because #1906 (comment)

there was no version pin for [sqlalchemy] in the pandas 1.x dependencies so you could easily end up installing pandas 1.x and SQLAlchemy 2.x,

@RobinL RobinL requested a review from ADBond January 30, 2024 11:09
@RobinL RobinL changed the title use explicit transactions, remove sqlalchemy version constraint SQLAlchemy 1.x and 2.x compatibility: Use explicit transactions, remove sqlalchemy version constraint Jan 30, 2024
Copy link
Contributor

@ADBond ADBond left a comment

Choose a reason for hiding this comment

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

This has got to pretty high up the list of lines changed to issues closed ratio!

Just a couple of things:

  • looks like there is still one more reference to .fetchall() in _table_exists_in_database here
  • would it be possible to update the changelog?

Happy for you to merge when ready

@ADBond
Copy link
Contributor

ADBond commented Jan 30, 2024

Also might be useful to fold these changes into splink4_dev relatively soon while it's fresh in the mind, so that the relevant changes get propagated to their correct new homes

@RobinL
Copy link
Member Author

RobinL commented Jan 30, 2024

This has got to pretty high up the list of lines changed to issues closed ratio!

Just a couple of things:

  • looks like there is still one more reference to .fetchall() in _table_exists_in_database here
  • would it be possible to update the changelog?

Happy for you to merge when ready

Good spots, fixed

@RobinL RobinL merged commit fcd6965 into master Jan 30, 2024
10 checks passed
@RobinL RobinL deleted the issue_1906_sqlalchemy_2_0 branch January 30, 2024 14:47
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.

Postgres backend fails when creating UDFs due to SQLAlchemy ROLLBACKs
2 participants