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 v1.4 (v2 API) #114

Merged
merged 1 commit into from
Sep 2, 2021

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Aug 30, 2021

Currently based on top of #113, supersedes #108

Following migration: https://docs.sqlalchemy.org/en/14/changelog/migration_20.html

  • Added SQLALCHEMY_WARN_20 environmental variable
  • Use future=True for engine and session creation (V1 -> v2 API)
  • query -> select (V1 -> v2 API)
  • Rename models.py -> database.py (models is too generic does not describe the module's purpose)
  • Moved get_session -> database.py (this method can be independent of the container)
  • Added mypy extension: https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html

I noted the count() method is now a bit more complex, but this is explained in: sqlalchemy/sqlalchemy#6794

A few things to double-check:

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #114 (848a95b) into develop (c0a67a3) will decrease coverage by 0.12%.
The diff coverage is 97.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #114      +/-   ##
===========================================
- Coverage    99.68%   99.56%   -0.13%     
===========================================
  Files            7        7              
  Lines         1585     1601      +16     
===========================================
+ Hits          1580     1594      +14     
- Misses           5        7       +2     
Impacted Files Coverage Δ
disk_objectstore/container.py 99.39% <95.34%> (-0.25%) ⬇️
disk_objectstore/database.py 100.00% <100.00%> (ø)
disk_objectstore/utils.py 99.60% <100.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0a67a3...848a95b. Read the comment docs.

Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

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

Everything great here 👍🏽 I just have one question out of curiosity but this is ready to merge for me.

Comment on lines +1854 to +1857
session.execute(text("COMMIT"))
session.execute(text("VACUUM"))
# ensure sqlalchemy knows to open a new transaction for the next execution
session.commit()
Copy link
Member

@ramirezfranciscof ramirezfranciscof Sep 2, 2021

Choose a reason for hiding this comment

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

What is the difference between session.execute(text("COMMIT")) and session.commit() and why is it necessary to make it differently here? (I imagine it has to do with the comment of different transactions but why)

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference is that this direct execution bypasses sqlalchemy’s auto-transaction mechanics, and stops it from starting a new transaction before executing the vacuum (which cannot be executed within a transaction)

@chrisjsewell
Copy link
Member Author

Cheers!

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