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

Make FTMstore cross-compatible with SQLAlchemy 1.4 and 2+ #32

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

catileptic
Copy link
Contributor

@catileptic catileptic commented Jul 24, 2023

  • run the SQLAlchemy tests with a Postgres connection
  • use the future=True flag in the create_engine method in order to use the SQLA 2+ API in SQLA 1.4 (as per the docs)

Without passing the future=True flag in the create_engine method, the following exception occurs when running the tests with SQLAlchemy 1.4:

[...]
            with self.store.engine.connect() as conn:
                conn.execute(stmt)
>               conn.commit()
E               AttributeError: 'Connection' object has no attribute 'commit'

ftmstore/dataset.py:77: AttributeError
[...]
RemovedIn20Warning: Deprecated API features detected! These feature(s) are not compatible with SQLAlchemy 2.0. To prevent incompatible upgrades prior to updating applications, ensure requirements files are pinned to "sqlalchemy<2.0". Set environment variable SQLALCHEMY_WARN_20=1 to show all deprecation warnings.  Set environment variable SQLALCHEMY_SILENCE_UBER_WARNING=1 to silence this message. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    conn.execute(stmt)

@catileptic catileptic marked this pull request as draft July 24, 2023 15:17
@catileptic catileptic marked this pull request as ready for review August 8, 2023 13:41
self.engine = create_engine(database_uri, **config)
self.engine = create_engine(database_uri, future=True, **config)

Choose a reason for hiding this comment

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

requirements.txt still requires 2.0.x. That seems unintended?

Copy link
Contributor Author

@catileptic catileptic Aug 9, 2023

Choose a reason for hiding this comment

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

requirements.txt isn't used then running pip install. It's usually used for local dev or when a Docker image specifically runs pip install -r requirements.txt for a repo. In my view, it's fine is the version of SQLAlchemy stays pinned in requirements.txt, especially if it's pinned to v2, since that's ultimately what we want all these libs to use.

LE: setup.py in our case is what is used when running pip install (or when building a lib and uploading it to PyPI).

Choose a reason for hiding this comment

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

Ah, sorry, I checked setup.py but that doesn’t specify any dependencies at all and I had assumed it would fall back to requirements.txt in that case of something like that. But I just tried this out in a fresh venv…

pip3 install git+https://github.com/alephdata/followthemoney-store.git@chore/sqla-1.4-2-compatibility
python3 -c "python3 -c "import sqlalchemy; print(sqlalchemy.__version__)"

… and it turns out that SQLAlchemy isn’t installed at all in this case and would need to be installed manually. I’m fine with this PR then, but maybe we should consider specifying dependencies in setup.py (either in this PR or in a separate PR)?

@catileptic catileptic merged commit c486356 into main Aug 9, 2023
1 check passed
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