feat(async): add support for async sessions #350
Conversation
|
Thank you! I'll have a look at this in the next days. |
There was a problem hiding this comment.
Sorry for replying a bit later. I've taken an initial look at your PR. Using AsyncSession with this implementation seems to be working great and might bring advantages to the users with supporting web frameworks.
However, the current PR would break things in synchronous execution contexts. We still have to support these since many relying projects use frameworks like flask 1.x. I have added a few comments to the files and I'm looking forward to your ideas on how to solve these challenges!
I have not yet taken a look at the tests since they are failing here. I think before fixing them, we should discuss my points. Additionally, I'd prefer the tests to remain mainly synchronous since that's the major use of this library right now. Async tests should complement the current tests, not replace them.
Looking forward to your reply!
Codecov ReportBase: 97.09% // Head: 96.24% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #350 +/- ##
==========================================
- Coverage 97.09% 96.24% -0.85%
==========================================
Files 7 9 +2
Lines 722 879 +157
==========================================
+ Hits 701 846 +145
- Misses 21 33 +12
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 at Codecov. |
|
Looks like everything is coming together, I'll check the new changes out soon! Thank you for all the effort 🙂 @Fred7b do you also have time for a full review? If I recall correctly, u originally created the issue so I would appreciate the input from you! |
|
A polite ping 🙂 |
|
@jendrikjoe sorry, I've been focused on other issues. My plan is to merge those first, release a new beta with the other issues and then immediately release another beta including async session support so both versions can be tried out, in case async session support still has some bugs we didn't find. If there's anything left to change I'll let you know in the next days :) |
|
@erikwrede no worries and thanks for explaining 🤗 The release plan makes a lot of sense and take all the time you need for it 👍 Just wanted to make sure that the MR is not forgotten 😉 |
|
Quick Update: Release of this is pushed back until #355 is merged. |
|
No worries @erikwrede 🤗 Just ping me whenever I should resolve conflicts here 👍 |
|
Hey @jendrikjoe |
|
Thanks for pinging me 👍 Will try resolve the conflicts later today 🙂 |
|
Great! I noticed that some of the original tests are still modified to use async methods. I'd prefer that entire part to be separate - leave the sync tests and Schema resolvers as is and create separate object types with async resolvers and corresponding tests. Otherwise a test failure will not tell wether the failure hints at a fundamental problem or problems with the async extension. What do you think? |
|
I parametrize the schema now to have a synchronous and an asynchronous for all relevant tests. This way every test reports with which schema it failed, while still avoiding to copy all the tests. Or are you referring to the changes I made on the I still have to investigate why the batching tests using I as well moved some assert statements in the batching tests. I had some bugs in them, which led to the result of querying the schema having an error and then of course wrong sql statement counts, too. However, first testing that no errors occur when calling the schema, before counting the SQL statements made the debugging a lot easier and the error more apparent. |
|
Will try to figure as well what the issues with the session fixture is 🙈 |
|
Should be all working now 🥳 |
graphene_sqlalchemy/fields.py
Outdated
| @@ -81,8 +84,51 @@ def get_query(cls, model, info, sort=None, **args): | |||
|
|
|||
| @classmethod | |||
| def resolve_connection(cls, connection_type, model, info, args, resolved): | |||
| session = get_session(info.context) | |||
| if resolved is None: | |||
| if not is_sqlalchemy_version_less_than("1.4") and isinstance( | |||
There was a problem hiding this comment.
I don't think we need to check this every request, it will just introduce inefficiencies in every request. Maybe a "startup check" or documentation does the job here.
There was a problem hiding this comment.
Maybe changing lines 16/17 so that AsyncSession has a defined value (None), if sqlalchemy_version < 1.4 will be more elegant
There was a problem hiding this comment.
The problem here is the isinstance check. The import of it only happens if the version is nor less than 1.4. So in all other versions I need to fail the if statement before doing the isinstance check. I can imagine to ways around:
- do the startup check as you described and store the result in a constant
SQLALCHEMY_LESS_THAN_1_4, so I just need to check the boolean value of the constant - Invert this statement to check if it is a synchronous session
I would prefer the first option, as I am not sure how to check if a session is sync in sqlalchemy. There seems to be a protected class attribute _is_asyncio with sessions, but checking that would probably agin require checking the version of sqlalchemy first as I assume the property is new 😉
There was a problem hiding this comment.
Startup check option sounds great!
There was a problem hiding this comment.
We'll need this anyways as 2.0 is in beta; so we should use 1.4 style on all queries if possible - even in sync session. Scope of a different PR though. This will also require the global option.
There was a problem hiding this comment.
Implemented the startup option 👍
| assert select_statements[-2].startswith("SELECT reporters_1.id") | ||
| assert "WHERE reporters_1.id IN" in select_statements[-2] | ||
| if async_session: | ||
| assert len(select_statements) == 2 # TODO: Figure out why async has less calls |
There was a problem hiding this comment.
Yes the underlying reason is that with an async session, the count query is replaced by taking the length of the executed query. Definitely not ideal, but as all connections are created using connection_from_array_slice, all seem to anyhow query all elements and then filter by first, last, before and after 🤔
I would replace the comment here with the actual reason and would maybe then raise a separate MR about connections and how they are create, after understanding them better and having a discussion on what the roadmap for them is anyhow 🙂
| @benchmark | ||
| def execute_query(): | ||
| result = schema.execute( | ||
| async def execute_query(): | ||
| result = await schema.execute_async( | ||
| query, | ||
| context_value={"session": session_factory()}, | ||
| ) | ||
| assert not result.errors | ||
|
|
There was a problem hiding this comment.
This doesn't seem to be compatible with asyncio:
ionelmc/pytest-benchmark#66
I get warnings in this scheme, meaning that these tests don't really do what they should currently:
graphene_sqlalchemy/tests/test_benchmark.py:218: RuntimeWarning: coroutine 'benchmark_query.<locals>.execute_query' was never awaited
await benchmark_query(
There was a problem hiding this comment.
This could be a workaround: https://github.com/graphql-python/graphql-core/blob/1a5aca82eb1d026a8f48dd7c7d8f9c56b8cd651b/tests/benchmarks/test_execution_async.py#L41
Otherwise, maybe some of the solutions in the issue I linked could work as well. I'll try it out once I find some time
There was a problem hiding this comment.
Great catch 👍 I couldn't get the workaround running without adding nest_asyncio to the dev dependencies. I hope that is fine 🙂
| @pytest.mark.asyncio | ||
| async def test_custom_identifier(session): | ||
| await add_test_data(session) |
There was a problem hiding this comment.
I get the following warning here (doesn't appear without this PR):
graphene_sqlalchemy/tests/test_query.py::test_custom_identifier[False]
/Users/erik/.pyenv/versions/graphene-sqalchemy/lib/python3.10/site-packages/_pytest/unraisableexception.py:78: PytestUnraisableExceptionWarning: Exception ignored in: <coroutine object SQLAlchemyObjectType.get_node.<locals>.get_result at 0x11496bae0>
Traceback (most recent call last):
File "/Users/erik/.pyenv/versions/graphene-sqalchemy/lib/python3.10/site-packages/graphql/language/visitor.py", line 324, in get_enter_leave_for_kind
return self.enter_leave_map[kind]
KeyError: 'operation_definition'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/erik/.pyenv/versions/graphene-sqalchemy/lib/python3.10/site-packages/graphql/language/visitor.py", line 136, in get_enter_leave_for_kind
return self.enter_leave_map[kind]
KeyError: 'operation_definition'
There was a problem hiding this comment.
That is interesting, I cannot reproduce that locally 🤔 Given your output, I tried to reproduce it using tox -e py310-sql14 🙂
Signed-off-by: Erik Wrede <erikwrede2@gmail.com>
This is my initial suggestion on how to support async sessions in graphene-sqlalchemy as discussed in #324 🙂
It is not completely done yet, but I thought I would already open it to give you a chance for some feedback.
Sorry as well for all the style changes. My IDE ran black over it. I can revert those changes, however I couldn't find anything in the contribution guidelines regarding the style, so I thought I keep it for the time being 🤷♂️
As mentioned in the original issue: Async only supports joined and selectin loads and no lazy ones. This should be stated somewhere in the docs, I assume 😉
Open points: