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

Dependencies: Update to sqlalchemy~=2.0 #6146

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Oct 12, 2023

A number of minor changes were required for the update:

  • Queries that use order_by now need to include the property that is
    being ordered on in the list of projections.
  • The Session.bulk_update_mappings and Session.bulk_insert_mappings
    are replaced by using Session.execute with the update and insert
    methods.
  • The sqlalchemy-utils dependency is no longer used as well as the
    tests/storage/psql_dos/test_utils.py file that used it.
  • The future=True is removed from the engine creation. This was a
    temporary flag to enable v2.0 compatibility with v1.4.
  • Test of schema equivalence for export archives needed to be updated
    since the casting of UUID columns for PostgreSQL changed.
  • Remove the sphinx-sqlalchemy dependency since it is not compatible
    with sqlalchemy~=2.0. Temporarily comment out its usage in docs.

@sphuber sphuber force-pushed the feature/sqlalchemy-2.0 branch 3 times, most recently from 92656b5 to 2386b18 Compare October 17, 2023 08:46
Copy link
Member

@edan-bainglass edan-bainglass left a comment

Choose a reason for hiding this comment

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

A few general questions:

  1. What's with all the mypy/pylint overrides?
  2. Lots of failed tests at the bottom. Plan?

For 1, I see that you also updated the sqlalchemy[mypy] pre-commit dependency? Related?

aiida/storage/psql_dos/alembic_cli.py Show resolved Hide resolved
aiida/storage/psql_dos/backend.py Show resolved Hide resolved
aiida/storage/psql_dos/utils.py Show resolved Hide resolved
tests/tools/archive/test_schema.py Show resolved Hide resolved
@sphuber
Copy link
Contributor Author

sphuber commented Nov 7, 2023

Thanks for the review @edan-bainglass

What's with all the mypy/pylint overrides?

Typing changed a lot in v2.0 (see below). There are some issues where these lead to false positives: sqlalchemy/sqlalchemy#7726
There may be solutions, but I don't have time to work on those now and they are not worth the investment frankly.

Lots of failed tests at the bottom. Plan?

Do you mean the red crosses at the end of the changes in the github interface? Those are not failed tests. All tests pass (see bottom of this PR where there is a box of all checks and it says "All checks have passed"). They are just "annotations" that Github adds in scanning the code. They are quite dumb since it just does some static analysis and trips over raise RuntimeErrors in the code, but these are added intentionally in the tests. You can ignore them, they are there in every PR.

For 1, I see that you also updated the sqlalchemy[mypy] pre-commit dependency? Related?

For v2.0, we no longer need a separate sqlalchemy-stubs2 package for typing stubs. Just need mypy installed. This can be installed with the mypy extra of sqlalchemy: https://docs.sqlalchemy.org/en/20/orm/extensions/mypy.html#installation
We already install it ourselves, but I still add, because in this way we are sure that our requirement is compatible with that of sqlalchemy.

@sphuber sphuber force-pushed the feature/sqlalchemy-2.0 branch 4 times, most recently from edba2e5 to 6489123 Compare November 7, 2023 21:19
Copy link
Member

@edan-bainglass edan-bainglass left a comment

Choose a reason for hiding this comment

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

@sphuber Thanks for answering my previous questions.

LGTM!

A number of minor changes were required for the update:

* Queries that use `order_by` now need to include the property that is
  being ordered on in the list of projections.
* The `Session.bulk_update_mappings` and `Session.bulk_insert_mappings`
  are replaced by using `Session.execute` with the `update` and `insert`
  methods.
* The `sqlalchemy-utils` dependency is no longer used as well as the
  `tests/storage/psql_dos/test_utils.py` file that used it.
* The `future=True` is removed from the engine creation. This was a
  temporary flag to enable v2.0 compatibility with v1.4.
* Test of schema equivalence for export archives needed to be updated
  since the casting of `UUID` columns for PostgreSQL changed.
* Remove the `sphinx-sqlalchemy` dependency since it is not compatible
  with `sqlalchemy~=2.0`. The documentation that relied on it to show
  the database models is temporarily commented out.
@sphuber sphuber merged commit a216f50 into aiidateam:main Nov 8, 2023
36 checks passed
@sphuber sphuber deleted the feature/sqlalchemy-2.0 branch November 8, 2023 13:17
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