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

UPDATE: SQLAlchemy 2.0 #93

Merged
merged 5 commits into from
Apr 21, 2023
Merged

Conversation

jzluo
Copy link
Contributor

@jzluo jzluo commented Mar 17, 2023

As mentioned in #92, SQLAlchemy is pinned to <1.5. All tests pass with SQLAlchemy 2.0.6 - the only SQLAlchemy warning is:

jupyter_cache/cache/db.py:17
  /home/jon/projects/jupyter-cache/jupyter_cache/cache/db.py:17: MovedIn20Warning: The ``declarative_base()`` 
function is now available as sqlalchemy.orm.declarative_base(). (deprecated since: 1.4) (Background on 
SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    OrmBase = declarative_base()

because of https://docs.sqlalchemy.org/en/20/changelog/migration_14.html#change-5508. sqlalchemy.ext.declarative.declarative_base is still present.

@welcome
Copy link

welcome bot commented Mar 17, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@jzluo
Copy link
Contributor Author

jzluo commented Mar 28, 2023

@choldgraf apologies for the ping - I saw that you were the reviewer for a similar PR about SQLAlchemy 1.4. Do you have an estimate for when someone can check this PR or create a different solution? Currently projects can't use myst-nb and SQLAlchemy 2.

@jzluo
Copy link
Contributor Author

jzluo commented Apr 13, 2023

@chrisjsewell could we get eyes on this? Thank you

@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (47e90f4) 83.32% compared to head (894bb29) 83.30%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
- Coverage   83.32%   83.30%   -0.02%     
==========================================
  Files          20       20              
  Lines        1355     1354       -1     
==========================================
- Hits         1129     1128       -1     
  Misses        226      226              
Flag Coverage Δ
pytests 83.30% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jupyter_cache/cache/db.py 85.39% <100.00%> (-0.06%) ⬇️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

choldgraf
choldgraf previously approved these changes Apr 14, 2023
Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

LGTM as long as the tests are happy 👍 . Does sqalchemy often make breaking changes at minor versions? If not, then why not pin it to <3.0?

@jzluo
Copy link
Contributor Author

jzluo commented Apr 14, 2023

LGTM as long as the tests are happy 👍 . Does sqalchemy often make breaking changes at minor versions? If not, then why not pin it to <3.0?

Thanks! Was just following the PR for supporting 1.4 where you mentioned preference for pinning to minor versions. I'm not sure if sqlalchemy had a history of making breaking changes at minor versions, but I think pinning to <3.0 should be fine. What do you think would be best?

@jzluo
Copy link
Contributor Author

jzluo commented Apr 20, 2023

@choldgraf @chrisjsewell sorry for the ping again - I think it's important to resolve this quickly as it prevents projects that use myst-nb from using sqlalchemy 2.0.

@choldgraf
Copy link
Member

I'll try to take a look this weekend if I have a moment. In the meantime it'd be helpful if you made the modifications to the codebase to avoid the new warning that pops up. Not crucial but it'd be helpful since we'll need to do that at some point anyway

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

LGTM thanks! Appreciate the extra fix to get rid of the warning 👍

@choldgraf choldgraf changed the title Allow SQLAlchemy 2.0 UPDATE: SQLAlchemy 2.0 Apr 21, 2023
@choldgraf choldgraf merged commit 84c3530 into executablebooks:master Apr 21, 2023
9 checks passed
@welcome
Copy link

welcome bot commented Apr 21, 2023

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@jzluo
Copy link
Contributor Author

jzluo commented Apr 21, 2023

Thank you Chris!

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Apr 22, 2023

Updating jupyter_cache from 0.5.0 to 0.6.0 breaks the “stable (ubuntu-latest, 3.8, pip, doc)” CI test of bids-standard/pybids with:

ImportError: cannot import name 'declarative_base' from 'sqlalchemy.orm' (/home/runner/work/pybids/pybids/virtenv/lib/python3.8/site-packages/sqlalchemy/orm/__init__.py)

See https://github.com/bids-standard/pybids/actions/runs/4767649000/jobs/8476115124

It is unclear from the logs which version of SQLAlchemy we use. Most probably python3-sqlalchemy 1.4.31 which is bundled with Ubuntu 22.04.

@choldgraf
Copy link
Member

choldgraf commented Apr 22, 2023

Ah i bet you're using old version of sqlalchemy. Given that sqlalchemy 2 is still less than a year old we should try to support both for a bit. We should have used a try/except block for the import instead of just changing it. If somebody wants to make a quick PR (@jzluo ?) Im happy to review and cut a quick patch release

@DimitriPapadopoulos
Copy link
Contributor

You're correct, from pybids/pyproject.toml:

  "sqlalchemy <1.4.0.dev0",

Perhaps we should update. I'll look into why we need SQLAlchemy < 1.4.0.

@DimitriPapadopoulos
Copy link
Contributor

In the meantime, see PR #105 for compatibility with SQLAlchemy 1.3.

DimitriPapadopoulos added a commit to DimitriPapadopoulos/pybids that referenced this pull request Apr 22, 2023
We require SQLAlchemy < 1.4.0.

However, jupyter_cache 0.6.0 relies on SQLAlchemy >= 1.4.0:
executablebooks/jupyter-cache#93

Avoid pulling jupyter_cache 0.6.0 for now.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/pybids that referenced this pull request Apr 22, 2023
We require SQLAlchemy < 1.4.0.

However, jupyter_cache 0.6.0 relies on SQLAlchemy >= 1.4.0:
executablebooks/jupyter-cache#93

Avoid pulling jupyter_cache 0.6.0 for now.
hwchase17 pushed a commit to langchain-ai/langchain that referenced this pull request Apr 25, 2023
With executablebooks/jupyter-cache#93 merged and
`MyST-NB` updated, we can now support SQLAlchemy 2. Closes #1766
vowelparrot pushed a commit to langchain-ai/langchain that referenced this pull request Apr 26, 2023
With executablebooks/jupyter-cache#93 merged and
`MyST-NB` updated, we can now support SQLAlchemy 2. Closes #1766
vowelparrot pushed a commit to langchain-ai/langchain that referenced this pull request Apr 28, 2023
With executablebooks/jupyter-cache#93 merged and
`MyST-NB` updated, we can now support SQLAlchemy 2. Closes #1766
samching pushed a commit to samching/langchain that referenced this pull request May 1, 2023
With executablebooks/jupyter-cache#93 merged and
`MyST-NB` updated, we can now support SQLAlchemy 2. Closes langchain-ai#1766
yanghua pushed a commit to yanghua/langchain that referenced this pull request May 9, 2023
With executablebooks/jupyter-cache#93 merged and
`MyST-NB` updated, we can now support SQLAlchemy 2. Closes langchain-ai#1766
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.

None yet

3 participants