FIX: Adapt to SQLAlchemy 1.4+ #985
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #985 +/- ##
==========================================
- Coverage 83.41% 83.37% -0.04%
==========================================
Files 38 38
Lines 4287 4290 +3
Branches 1100 1100
==========================================
+ Hits 3576 3577 +1
- Misses 515 517 +2
Partials 196 196
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
effigies
changed the title
|
Passing locally. Commits have informative messages. |
| try: | ||
| from sqlalchemy.orm import declarative_base | ||
| except ImportError: # sqlalchemy < 1.4 | ||
| from sqlalchemy.ext.declarative import declarative_base |
There was a problem hiding this comment.
sweet! Thank you @effigies . Since it is somewhat of an explicit "support old and new" - may be worth adding a CI run which would test with sqlalchemy 1.3.24 ?
or since it has been years since 1.4 release, just require >= 1.4?
There was a problem hiding this comment.
You'll have to wait for a new release of jupyter_cache that will restore support of sqlalchemy 1.3. Otherwise handle complex dependencies:
- latest
mist-nb⇒jupyter_cache0.6.0 ⇒sqlalchemy≥ 1.4 sqlalchemy< 1.4⇒previousmist-nbor upcomingmist-nbthat would pull upcomingjupyter_cachethat would restore support forsqlalchemy1.3.
There was a problem hiding this comment.
I've generally wanted an explicit test of minimum dependencies, but never got around to it. It seems bad to go from requiring <1.4 to >=1.4 in a single commit.
There was a problem hiding this comment.
I created #987 to add a min-requirements.txt file and test it. It is currently passing locally but failing with what is very likely an out-of-memory error on GHA. How about we continue this conversation there?
Looking at executablebooks/jupyter-cache#93 (thanks for finding, @DimitriPapadopoulos!), our sqlalchemy issues may have been smaller than they seemed they were going to be. Let's see.
Closes #680.