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 databases and SQLAlchemy #5799

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

musicinmybrain
Copy link
Contributor

@musicinmybrain musicinmybrain commented Dec 18, 2022

Incompatibilities with recent SQLAlchemy versions are fixed in databases 0.7.0. By requiring databases[sqlite] >=0.7.0,<0.8.0 for testing, we can stop pinning an old SQLAlchemy version, instead requiring sqlalchemy >=1.4.42,<1.5.

Fixes #5556.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 574b0a2 at: https://639f3fe2d8175d240d2d6490--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2023

📝 Docs preview for commit bd2295f at: https://63b9f42d42bbfe7af491a2cf--fastapi.netlify.app

Copy link
Sponsor

@Ryandaydev Ryandaydev left a comment

Choose a reason for hiding this comment

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

Checked out this PR, installed updates, and ran tests.

Test failing:
FAILED tests/test_tutorial/test_async_sql_databases/test_tutorial001.py::test_create_read - sqlalchemy.exc.RemovedIn20Warning: Deprecated API features detected! These feature(s) are not compatib...

sqlalchemy.exc.RemovedIn20Warning: Deprecated API features detected! These feature(s) are not compatible with SQLAlchemy 2.0. To prevent incompatible upgrades prior to updating applications, ensure requirements files are pinned to "sqlalchemy<2.0". Set environment variable SQLALCHEMY_WARN_20=1 to show all deprecation warnings. Set environment variable SQLALCHEMY_SILENCE_UBER_WARNING=1 to silence this message. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)

@musicinmybrain
Copy link
Contributor Author

Thanks for looking at it. Those warnings were added in sqlalchemy 1.4.46, which was released after I opened this PR. I see three possible courses of action:

  1. Since the version specification in the PR does not permit SQLAlchemy 2.0, ignore these forward-compatibility warnings (by filtering them in pyproject.toml or by setting the environment variable SQLALCHEMY_SILENCE_UBER_WARNING=1 to suppress them).
  2. Pin SQLAlchemy to <1.4.46; but this also keeps users from benefiting from the bugfixes in an otherwise-compatible release.
  3. Find the places where deprecated API features are used, and adjust them to be forward-compatible with SQLAlchemy 2.0.

I’ll take a closer look at this and see if there is an obvious way to do option 3.

@musicinmybrain
Copy link
Contributor Author

Here’s the full output:

====================================================== FAILURES ======================================================
__________________________________________________ test_create_read __________________________________________________

    def test_create_read():
        with TestClient(app) as client:
            note = {"text": "Foo bar", "completed": False}
            response = client.post("/notes/", json=note)
            assert response.status_code == 200, response.text
            data = response.json()
            assert data["text"] == note["text"]
            assert data["completed"] == note["completed"]
            assert "id" in data
>           response = client.get("/notes/")

tests/test_tutorial/test_async_sql_databases/test_tutorial001.py:129: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_e/lib64/python3.11/site-packages/starlette/testclient.py:488: in get
    return super().get(
_e/lib64/python3.11/site-packages/httpx/_client.py:1045: in get
    return self.request(
_e/lib64/python3.11/site-packages/starlette/testclient.py:454: in request
    return super().request(
_e/lib64/python3.11/site-packages/httpx/_client.py:821: in request
    return self.send(request, auth=auth, follow_redirects=follow_redirects)
_e/lib64/python3.11/site-packages/httpx/_client.py:908: in send
    response = self._send_handling_auth(
_e/lib64/python3.11/site-packages/httpx/_client.py:936: in _send_handling_auth
    response = self._send_handling_redirects(
_e/lib64/python3.11/site-packages/httpx/_client.py:973: in _send_handling_redirects
    response = self._send_single_request(request)
_e/lib64/python3.11/site-packages/httpx/_client.py:1009: in _send_single_request
    response = transport.handle_request(request)
_e/lib64/python3.11/site-packages/starlette/testclient.py:337: in handle_request
    raise exc
_e/lib64/python3.11/site-packages/starlette/testclient.py:334: in handle_request
    portal.call(self.app, scope, receive, send)
_e/lib64/python3.11/site-packages/anyio/from_thread.py:283: in call
    return cast(T_Retval, self.start_task_soon(func, *args).result())
/usr/lib64/python3.11/concurrent/futures/_base.py:456: in result
    return self.__get_result()
/usr/lib64/python3.11/concurrent/futures/_base.py:401: in __get_result
    raise self._exception
_e/lib64/python3.11/site-packages/anyio/from_thread.py:219: in _call_func
    retval = await retval
fastapi/applications.py:270: in __call__
    await super().__call__(scope, receive, send)
_e/lib64/python3.11/site-packages/starlette/applications.py:124: in __call__
    await self.middleware_stack(scope, receive, send)
_e/lib64/python3.11/site-packages/starlette/middleware/errors.py:184: in __call__
    raise exc
_e/lib64/python3.11/site-packages/starlette/middleware/errors.py:162: in __call__
    await self.app(scope, receive, _send)
_e/lib64/python3.11/site-packages/starlette/middleware/exceptions.py:79: in __call__
    raise exc
_e/lib64/python3.11/site-packages/starlette/middleware/exceptions.py:68: in __call__
    await self.app(scope, receive, sender)
fastapi/middleware/asyncexitstack.py:21: in __call__
    raise e
fastapi/middleware/asyncexitstack.py:18: in __call__
    await self.app(scope, receive, send)
_e/lib64/python3.11/site-packages/starlette/routing.py:706: in __call__
    await route.handle(scope, receive, send)
_e/lib64/python3.11/site-packages/starlette/routing.py:276: in handle
    await self.app(scope, receive, send)
_e/lib64/python3.11/site-packages/starlette/routing.py:66: in app
    response = await func(request)
fastapi/routing.py:255: in app
    content = await serialize_response(
fastapi/routing.py:131: in serialize_response
    value, errors_ = field.validate(response_content, {}, loc=("response",))
pydantic/fields.py:895: in pydantic.fields.ModelField.validate
    ???
pydantic/fields.py:928: in pydantic.fields.ModelField._validate_sequence_like
    ???
pydantic/fields.py:1094: in pydantic.fields.ModelField._validate_singleton
    ???
pydantic/fields.py:884: in pydantic.fields.ModelField.validate
    ???
pydantic/fields.py:1101: in pydantic.fields.ModelField._validate_singleton
    ???
pydantic/fields.py:1148: in pydantic.fields.ModelField._apply_validators
    ???
pydantic/class_validators.py:318: in pydantic.class_validators._generic_validator_basic.lambda13
    ???
pydantic/main.py:717: in pydantic.main.BaseModel.validate
    ???
<string>:2: in keys
    ???
_e/lib64/python3.11/site-packages/sqlalchemy/util/deprecations.py:467: in warned
    _warn_with_version(message, version, wtype, stacklevel=3)
_e/lib64/python3.11/site-packages/sqlalchemy/util/deprecations.py:47: in _warn_with_version
    _emit_uber_warning(type_, stacklevel)
_e/lib64/python3.11/site-packages/sqlalchemy/util/deprecations.py:105: in _emit_uber_warning
    _warnings_warn(warn, stacklevel=stacklevel + 1)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

message = RemovedIn20Warning('Deprecated API features detected! These feature(s) are not compatible with SQLAlchemy 2.0. To prev...o show all deprecation warnings.  Set environment variable SQLALCHEMY_SILENCE_UBER_WARNING=1 to silence this message.')
category = None, stacklevel = 5

    def _warnings_warn(message, category=None, stacklevel=2):
    
        # adjust the given stacklevel to be outside of SQLAlchemy
        try:
            frame = sys._getframe(stacklevel)
        except ValueError:
            # being called from less than 3 (or given) stacklevels, weird,
            # but don't crash
            stacklevel = 0
        except:
            # _getframe() doesn't work, weird interpreter issue, weird,
            # ok, but don't crash
            stacklevel = 0
        else:
            # using __name__ here requires that we have __name__ in the
            # __globals__ of the decorated string functions we make also.
            # we generate this using {"__name__": fn.__module__}
            while frame is not None and re.match(
                r"^(?:sqlalchemy\.|alembic\.)", frame.f_globals.get("__name__", "")
            ):
                frame = frame.f_back
                stacklevel += 1
    
        if category is not None:
            warnings.warn(message, category, stacklevel=stacklevel + 1)
        else:
>           warnings.warn(message, stacklevel=stacklevel + 1)
E           sqlalchemy.exc.RemovedIn20Warning: Deprecated API features detected! These feature(s) are not compatible with SQLAlchemy 2.0. To prevent incompatible upgrades prior to updating applications, ensure requirements files are pinned to "sqlalchemy<2.0". Set environment variable SQLALCHEMY_WARN_20=1 to show all deprecation warnings.  Set environment variable SQLALCHEMY_SILENCE_UBER_WARNING=1 to silence this message. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)

_e/lib64/python3.11/site-packages/sqlalchemy/util/langhelpers.py:1679: RemovedIn20Warning
============================================== short test summary info ===============================================
FAILED tests/test_tutorial/test_async_sql_databases/test_tutorial001.py::test_create_read - sqlalchemy.exc.RemovedIn20Warning: Deprecated API features detected! These feature(s) are not compatible with SQL...
===================================== 1 failed, 1400 passed in 90.00s (0:01:30) ======================================

Unfortunately, this doesn’t carry any information about which deprecated API feature was used, and it’s hard to even be sure where the deprecated usage is happening.

I tried setting 'always::DeprecationWarning', in tool.pytest.ini_options.filterwarnings in pyproject.toml, and got this, which is much briefer but more helpful:

tests/test_tutorial/test_async_sql_databases/test_tutorial001.py::test_create_read
  /home/ben/src/forks/fastapi/fastapi/routing.py:131: RemovedIn20Warning: Deprecated API features detected! These feature(s) are not compatible with SQLAlchemy 2.0. To prevent incompatible upgrades prior to updating applications, ensure requirements files are pinned to "sqlalchemy<2.0". Set environment variable SQLALCHEMY_WARN_20=1 to show all deprecation warnings.  Set environment variable SQLALCHEMY_SILENCE_UBER_WARNING=1 to silence this message. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    value, errors_ = field.validate(response_content, {}, loc=("response",))

That points at an incompatibility in FastAPI, and it is something specific enough I can reasonably filter it out.

@musicinmybrain
Copy link
Contributor Author

Added 'ignore::sqlalchemy.exc.RemovedIn20Warning:fastapi', to tool.pytest.ini_options.filterwarnings in pyproject.toml, along with a comment justifying the filtering.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 2deb233 at: https://63c5eb1e4a9d912fcb4f5299--fastapi.netlify.app

Copy link
Sponsor

@Ryandaydev Ryandaydev left a comment

Choose a reason for hiding this comment

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

After warning suppressed, tests are passing.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2023

📝 Docs preview for commit 656ce50 at: https://64038cb82bd72c60f5c3e0c2--fastapi.netlify.app

@musicinmybrain
Copy link
Contributor Author

Rebased on master again.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 262fa51 at: https://640c9640611f6123320901d1--fastapi.netlify.app

@musicinmybrain
Copy link
Contributor Author

Rebased on master again.

@github-actions
Copy link
Contributor

📝 Docs preview for commit c4a4592 at: https://64722e9cae27a515156fe6e0--fastapi.netlify.app

@musicinmybrain
Copy link
Contributor Author

Rebased on master again.

@tiangolo
Copy link
Owner

📝 Docs preview for commit 3017463 at: https://64930f371649720e5435aa03--fastapi.netlify.app

@musicinmybrain musicinmybrain force-pushed the sqlalchemy-databases branch 3 times, most recently from 7c97a7d to 2be88d5 Compare July 1, 2023 22:13
@musicinmybrain musicinmybrain force-pushed the sqlalchemy-databases branch 2 times, most recently from c08d33d to 7bc2ffb Compare July 12, 2023 20:27
@musicinmybrain
Copy link
Contributor Author

Rebased again.

@musicinmybrain
Copy link
Contributor Author

@tiangolo, any interest in merging this? I’ve been carrying it as a downstream patch in Fedora Linux for quite a while, and it seems like a straighforward improvement that would allow CI testing with current SQLAlchemy releases in the 1.4.x series.

@musicinmybrain
Copy link
Contributor Author

Rebased again, and adjusted to allow databases 0.8.x too (databases[sqlite] >=0.7.0,<0.9.0).

@jonocodes
Copy link

Can you clarify. Is SqlAlchemy 2.0 not supported yet? Or is that just a limitation of testing?

@musicinmybrain
Copy link
Contributor Author

Can you clarify. Is SqlAlchemy 2.0 not supported yet? Or is that just a limitation of testing?

These are only test dependencies.

The actual FastAPI package doesn’t import sqlalchemy, but some of the tests (and examples in the documentation, which can be run as tests) do. The databases usage is only in docs_src/async_sql_databases/tutorial001.py, but that means databases constrains the version of sqlalchemy used for testing.

This PR predates the SQLAlchemy 2.0.0 release, so updating to databases 0.7.0 allowed tests to run on the then-latest version of sqlalchemy. Now databases is at 0.8.0, which needs sqlalchemy>=1.4.42,<1.5. So databases is again blocking testing with the latest version of SQLAlchemy, and I should probably edit this PR to restore the original comment about removing the databases dependency in order to upgrade SQLAlchemy.

Still, even if it wouldn’t allow testing with SQLAlchemy 2.0, this PR would at least improve the status quo.

@musicinmybrain
Copy link
Contributor Author

[…] I should probably edit this PR to restore the original comment about removing the databases dependency in order to upgrade SQLAlchemy.

Rebased on master, and the PR no longer removes the mentioned comment.

@musicinmybrain
Copy link
Contributor Author

Added 'ignore::sqlalchemy.exc.RemovedIn20Warning:fastapi', to tool.pytest.ini_options.filterwarnings in pyproject.toml, along with a comment justifying the filtering.

It looks like this is no longer necessary in the current version of FastAPI, so I dropped the second commit.

For posterity, since I’m force-pushing again, that commit looked like:

From 6f11a91c4c60914075a5ca36594be499c524f71a Mon Sep 17 00:00:00 2001
From: "Benjamin A. Beasley" <code@musicinmybrain.net>
Date: Mon, 16 Jan 2023 19:19:56 -0500
Subject: [PATCH] Ignore SQLAlchemy RemovedIn20Warning from FastAPI

We have SQLAlchemy pinned anyway; we are not yet concerned about
forward-compatibility with 2.0, especially in dependencies.
---
 pyproject.toml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pyproject.toml b/pyproject.toml
index 2870b31a..4aa9af44 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -97,6 +97,9 @@ filterwarnings = [
     "error",
     # TODO: needed by asyncio in Python 3.9.7 https://bugs.python.org/issue45097, try to remove on 3.9.8
     'ignore:The loop argument is deprecated since Python 3\.8, and scheduled for removal in Python 3\.10:DeprecationWarning:asyncio',
+    # We have SQLAlchemy pinned anyway; we are not yet concerned about
+    # forward-compatibility with 2.0, especially in dependencies.
+    'ignore::sqlalchemy.exc.RemovedIn20Warning:fastapi',
     'ignore:starlette.middleware.wsgi is deprecated and will be removed in a future release\..*:DeprecationWarning:starlette',
     # TODO: remove after upgrading HTTPX to a version newer than 0.23.0
     # Including PR: https://github.com/encode/httpx/pull/2309
-- 
2.41.0

@musicinmybrain
Copy link
Contributor Author

It looks like this is no longer necessary in the current version of FastAPI, so I dropped the second commit.

Hmm, the warning doesn’t show up in local testing, but still appears in the pydantic-v1 CI environments. I’ll restore the second commit to suppress the warning.

@musicinmybrain musicinmybrain force-pushed the sqlalchemy-databases branch 2 times, most recently from 260844a to 03cb89b Compare February 8, 2024 12:37
@musicinmybrain
Copy link
Contributor Author

Rebased on master again; added 'ignore::sqlalchemy.exc.RemovedIn20Warning:pydantic' since these warnings now arise from Pydantic as well.

Incompatibilities with recent SQLAlchemy versions are fixed in databases
0.7.0.
We have SQLAlchemy pinned anyway; we are not yet concerned about
forward-compatibility with 2.0, especially in dependencies.
@musicinmybrain
Copy link
Contributor Author

Rebased on master again.

@tiangolo
Copy link
Owner

📝 Docs preview for commit dca742e at: https://e0ca5caf.fastapitiangolo.pages.dev

@musicinmybrain
Copy link
Contributor Author

Now upgraded to allow current releases of databases and SQLAlchemy (0.9.x and 2.x, respectively).

Tests are still passing for me locally; we will see if the CI agrees.

@musicinmybrain
Copy link
Contributor Author

Trying to adjust the warning configuration so that this still works with Pydantic v1…

@tiangolo
Copy link
Owner

📝 Docs preview for commit 229748d at: https://b684d8ee.fastapitiangolo.pages.dev

@tiangolo
Copy link
Owner

📝 Docs preview for commit 25fe108 at: https://63296ba9.fastapitiangolo.pages.dev

@musicinmybrain
Copy link
Contributor Author

Now I’m seeing issues like:

E       AssertionError: {"detail":"Email already registered"}
E       assert 400 == 200
E        +  where 400 = <Response [400 Bad Request]>.status_code

and

sqlite3.OperationalError: attempt to write a readonly database

with Pydantic v1. I am not sure what to do about that. Suggestions welcome.

This is still useful as a downstream patch in Fedora, where we have Pydantic v2 only and would like to test with current versions of everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async SQL (Relational) Databases example in Docs is broken
4 participants