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

Upgrade SQLAlchemy to version 2 #415

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

Conversation

M4rque2
Copy link

@M4rque2 M4rque2 commented Mar 9, 2023

All unit tests passed

karlicoss and others added 5 commits January 30, 2023 19:20
exists is a method, so prior to change, assert would always pass

thankfully, this still works on sqlalchemy 1.4 after change
seems that table.table.exists() actually created table

if we replace it with self.db_has_table instead, it doesn't work
I assume because it works on sqlalchemy metadata level and not issuing actual sql engine commands

to demonstrate that I've put the asserts with self.db.has_table in setUp method

for now just removed the assert from test_create_table_shorthand* tests
@miraculixx
Copy link

miraculixx commented Mar 14, 2023

This is great! Solves #411 imho

@pudo Do you think this could be merged?

@qbit
Copy link

qbit commented Mar 17, 2023

It will also need a bump in setup.py, no?

Comment on lines +45 to +47

- uses: mxschmitt/action-tmate@v3

Copy link

Choose a reason for hiding this comment

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

looks like you accidentally left a debugging step in github actions?

@brrttwrks
Copy link

Howdy. We are using a library that depends on dataset and would like to see what specifically is preventing this pull request and how we can help git it where it needs to be. Is this pull request adequate or what is holding it back?

@pudo pudo mentioned this pull request May 21, 2023
@pudo
Copy link
Owner

pudo commented May 21, 2023

Ok, so I've played with this a little bit on #420, and the problem is that this will only work on SQLite, not any DB with proper transactions management. SQLAlchemy has fundamentally gotten rid of autocommit mode, which was underpinning dataset. While there's probably a way of faking autocommit mode on the library level, I wonder if the cleaner approach wouldn't be to actually make transaction management in dataset more explicit. It gets rid of some of the magicness, but would probably lead people towards using tx'en correctly and therefore creating much faster scripts.

@pudo
Copy link
Owner

pudo commented May 21, 2023

The more I dig into this the less certain I am that we're not due a massive re-work of the library in order to harmonize transaction handling and how it intersects with DDL commands. At the moment, the tests are producing a bunch of deadlocks when running against PostgreSQL.

Does anyone here really understand transactions, and how they relate to schema modification?

@miraculixx
Copy link

miraculixx commented Jun 20, 2023

@pudo

I wonder if the cleaner approach wouldn't be to actually make transaction management in dataset more explicit. It gets rid of some of the magicness (...)

I think the default should be autocommit, as that's what (IMHO) one would generally expect from a library like dataset. dataset's advantage is to avoid sqlalchemy's complexity, and I feel making transactions explicit as a default would take away from this. For more transaction control, dataset already provides a respective API, right? https://dataset.readthedocs.io/en/latest/quickstart.html#using-transactions

For the case where full transaction control and speed (and possibly explicit control over DDL) is a driving critiera, I suggest sqlalchemy native is a better choice.

@miraculixx
Copy link

miraculixx commented Jun 20, 2023

@pudo

The more I dig into this the less certain I am that we're not due a massive re-work of the library in order to harmonize transaction handling and how it intersects with DDL commands.

I added my thoughts , hope it helps (I don't know the library well so I may be wrong). Afaik DDL commands cannot be executed inside another transaction, at least that's what the alembic docs on autocommit_block() seem to imply:

This special directive is intended to support the occasional database DDL or system operation that specifically has to be run outside of any kind of transaction block. The PostgreSQL database platform is the most common target for this style of operation, as many of its DDL operations must be run outside of transaction blocks, even though the database overall supports transactional DDL.

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.

7 participants