Upgrade to sqlalchemy>=2 #1788
Upgrade to sqlalchemy>=2 #1788
Conversation
|
|
|
Failures are unrelated. |
nbgrader/api.py
Outdated
| from sqlalchemy.orm import (sessionmaker, scoped_session, relationship, | ||
| column_property) | ||
| column_property, declarative_base) |
There was a problem hiding this comment.
The preferred method for 2.0 is to use the DeclarativeBase class. This still works, but switching to the class makes a lot of sense in this PR.
see: https://docs.sqlalchemy.org/en/20/orm/mapping_api.html#sqlalchemy.orm.declarative_base
There was a problem hiding this comment.
Thank you for letting me know. I don't recall seeing a mention of this in the migration document (maybe I missed it), but I agree that it makes sense to switch. Will fix accordingly.
There was a problem hiding this comment.
@shreve @brichet DeclarativeBase is apparently a sqlalchemy 2.x addition (without being present in 1.4), so I should have bumped up the minimum version.
The previous declarative_base imported from sqlalchemy.orm should work with both 1.4 and 2.x.
The difference between them is for type-checkers, which at least right now is not a big deal for nbgrader. So I think it is worth going back to declarative_base over DeclarativeBase. What do you think?
There was a problem hiding this comment.
In light of #1796, I'll open a pr to revert the change to DeclarativeBase
There was a problem hiding this comment.
Thanks @tuncbkose, that looks good to me.
I don't really know how to be sure everything will work as before, not sure if the current tests cover it.
cc @jhamrick
| @@ -46,7 +46,7 @@ dependencies = [ | |||
| "python-dateutil>=2.8", | |||
| "rapidfuzz>=1.8", | |||
| "requests>=2.26", | |||
| "sqlalchemy>=1.4,<2", | |||
| "sqlalchemy>=1.4,<3", | |||
There was a problem hiding this comment.
Will it still work with sqlalchemy<2 ?
There was a problem hiding this comment.
The tests are still passing for me locally with sqlalchemy 1.4.46. That might be helpful to add to the test matrix if we want to continue supporting <2 moving forward.
There was a problem hiding this comment.
My understanding is that sqlalchemy==1.4 supports all the big changes for sqlalchemy==2. Only the future=True flags might change behavior (like this).
|
@tuncbkose can you please rebase it to check if all the tests pass ? |
|
Thanks @tuncbkose |
Most of the required changes so far were fairly innocent.
One of the warnings when running on
sqlalchemy<2was this one about changing cascade_backref behavior. It is still a bit unclear to me what consequences this change might have, but tests were passing onsqlalchemy<2with usingfuture=TrueinSessionandEngine, and they are passing now.