-
-
Notifications
You must be signed in to change notification settings - Fork 565
Defer #3819
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
base: main
Are you sure you want to change the base?
Defer #3819
Conversation
We need to un-hardcode the label
Reviewer's Guide by SourceryThis pull request introduces experimental incremental execution support in the GraphQL server, adds new e2e tests for Apollo and Relay clients, and updates the GraphiQL interface. It also includes various refactorings and enhancements across the codebase. Sequence diagram for incremental execution in GraphQL serversequenceDiagram
participant Client
participant Server
participant GraphQL
Client->>Server: Send GraphQL request
Server->>GraphQL: Execute request
alt Incremental Execution Enabled
GraphQL->>Server: Return initial result
Server->>Client: Send initial result
loop While more results
GraphQL->>Server: Return subsequent result
Server->>Client: Send subsequent result
end
else Standard Execution
GraphQL->>Server: Return final result
Server->>Client: Send final result
end
Class diagram for updated GraphQL executionclassDiagram
class GraphQLSchema {
+query: QueryType
+mutation: MutationType
+subscription: SubscriptionType
+directives: List<Directive>
}
class GraphQLIncrementalExecutionResults {
+initial_result: ExecutionResult
+subsequent_results: List<ExecutionResult>
}
class StrawberryConfig {
+enable_experimental_incremental_execution: bool
}
GraphQLSchema --> GraphQLIncrementalExecutionResults
GraphQLSchema --> StrawberryConfig
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Apollo Federation Subgraph Compatibility Results
Learn more: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3819 +/- ##
==========================================
- Coverage 95.04% 94.79% -0.25%
==========================================
Files 502 506 +4
Lines 32682 32824 +142
Branches 1695 1715 +20
==========================================
+ Hits 31061 31115 +54
- Misses 1348 1430 +82
- Partials 273 279 +6 🚀 New features to boost your workflow:
|
if isinstance(result, GraphQLIncrementalExecutionResults): | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to support this properly
strawberry/http/async_base_view.py
Outdated
# for Apollo | ||
# content type is `multipart/mixed;deferSpec=20220824,application/json` | ||
"path": ["blogPost"], | ||
"label": "relayTestsBlogPostQuery$defer$relayTestsCommentsFragment", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be hard-coded, in any case having both label and path seems to be ok for both relay and apollo, so we might want to keep them
tbd if we want to keep the .formatted instead of using our own wrappers
apparently there's no good way to define what spec a client is supporting (Apollo sends deferSpec=20220824
, but for relay it's up to whoever implements the fetch)
if self.config.enable_experimental_incremental_execution: | ||
directives = tuple(directives) + tuple(incremental_execution_directives) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to throw errors if user what experimental execution but we are not on graphql core 3.3+
# TODO: maybe here use the first result from incremental execution if it exists | ||
if isinstance(result, GraphQLExecutionResult) and result.errors: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what I meant with that comment :D
but we should probably get rid of the isinstance check?
@@ -0,0 +1,44 @@ | |||
import contextlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this from the previous location
"data": {"character": {"id": "1"}}, | ||
"hasNext": True, | ||
"pending": [{"path": ["character"], "id": "0"}], | ||
# TODO: check if we need this and how to handle it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could support these in a future version
@@ -4,8 +4,7 @@ | |||
import pytest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file was also moved
CodSpeed Performance ReportMerging #3819 will not alter performanceComparing Summary
|
Pre-release👋 Pre-release 0.266.0.dev.1744797470 [876d580] has been released on PyPi! 🚀 poetry add strawberry-graphql==0.266.0.dev.1744797470 |
/pre-release |
…efer`) To optimize the provided code, we can reduce the number of checks by combining related conditionals and accessing properties once instead of multiple times. Furthermore, we can streamline the creation of the `data` dictionary. Here is an optimized version of the code. In this optimized version. - We access `result.errors` and `result.extensions` once and store their values in the `errors` and `extensions` variables, respectively. - We use dictionary unpacking with conditionals to add the "errors" and "extensions" keys to `data` only when they are present. This should provide a minor performance improvement while maintaining the same functionality.
⚡️ Codeflash found optimizations for this PR📄 100% (1.00x) speedup for
|
/pre-release |
…efer`) (#3827) Co-authored-by: codeflash-ai[bot] <148906541+codeflash-ai[bot]@users.noreply.github.com>
This PR is now faster! 🚀 @patrick91 accepted my optimizations from: |
This PR is now faster! 🚀 codeflash-ai[bot] accepted my code suggestion above. |
@matclayton sorry, I totally missed that message! do you know if you can write a small reproduction? or was that fixed with GraphQL-core 3.3.0a7? |
Sorry I didn't follow up here, this is fixed in GraphQL core 3.3.0va7, btw we're running GraphQL-core 3.3.0v7 in prod now, and not seen a single issue so far. Need to still confirm the other bits of this PR, I'm hoping the team can take it off me the next week or so. |
b35cb92
to
876d580
Compare
/pre-release |
Another attempt :D
Summary by Sourcery
Implement experimental incremental execution in the GraphQL schema to support deferred and streamed responses. Refactor the async run method to handle incremental execution results. Add end-to-end tests for Apollo and Relay clients, and introduce a new CI workflow for running these tests.
New Features:
Enhancements:
CI:
Documentation:
Tests: