V7 support for request batching #3370
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fselmo
force-pushed
the
v7-support-for-request-batching
branch
5 times, most recently
from
2270e3f
to
f7114f4
Compare
|
This is good for an initial review. It's still missing documentation but that may be best in a separate PR? Thoughts there? Usage examples can be seen in the tests for now. |
fselmo
force-pushed
the
v7-support-for-request-batching
branch
2 times, most recently
from
809c2bd
to
27e3ed4
Compare
pacrob
reviewed
May 1, 2024
fselmo
commented
May 2, 2024
fselmo
force-pushed
the
v7-support-for-request-batching
branch
3 times, most recently
from
4fe1d57
to
3d343ca
Compare
Built-in types are not generics
- Implement some basic batching support for sync and async, running each request through request processing and response processing. - Achieve this through context manager that prevents request sending while batching is in progress and re-allows when batching is done. - Refactor batch middleware logic into Web3Middleware class - Fix up the sync batch method to work by using the ``w3._is_batching`` flag correctly. - Refactor the batching logic in the manager to be a bit more readable and more performant - even if ever so slightly. - Interestingly, using ``asyncio.gather()`` with x requests is still faster than batching the same amount of requests into a single batch request. More research into performance improvements is needed. - Iterate on performance improvement ideas for batching - Add basic tests for batch requests; minor typing & refactor
- When adding async requests to a batch for a batch request, handle the awaiting of extracting the request information in the manager instead of adding that overhead to the batch API. - Implement ``add_mapping()`` for batch requests
- Legacy websocket - IPC - Persistent connection providers (async_ipc, websocket)
- Move request batching to a higher level public API on the ``web3`` module; make the implementation methods on the manager private. - Create a proper ``AsyncWeb3ModuleTest`` class that can test differences between synchronous and asynchronous methods on the module that now exist. This gets rid of the parameterized tests that previously tested the static methods on both the ``Web3`` and the ``AsyncWeb3`` classes. - Tighten up typing related to batch requests; add unsupported methods for batching with tests
- Generalizaing name change: ``BatchRequestContextManager`` -> ``RequestBatcher`` since it is no longer just a context manager. - Add API to batcher for clearing (``clear``) and cancelling (``cancel``) the batch. - Add more tests around these changes.
- Only guarantee response `id` order for batch requests if every response has an `id` field. Otherwise, rely on the provider to return the responses in order and hope for the best. An `id` should really ever only be missing in very rare cases where the request object itself was not able to be parsed correctly. So, hopefully this proves to be robust enough for most use cases.
fselmo
force-pushed
the
v7-support-for-request-batching
branch
from
3d343ca
to
827788d
Compare
kclowes
approved these changes
May 9, 2024
There was a problem hiding this comment.
This is awesome! Feel free to take or leave any comments I left. I wonder if we make the RequestsInformation object a little more robust we could avoid all of the indexing - here for example. I was thinking maybe have RequestsInfo.method, RequestsInfo.params, RequestInfo.result_formatters, RequestInfo.error_formatters, etc. We could either pass around the object, or just whatever attributes each function needs. If that doesn't make sense, feel free to push back :)
fselmo
added a commit
to fselmo/web3.py
that referenced
this pull request
May 10, 2024
- unpack response formatters for better readability
- unpack response formatters for better readability
fselmo
force-pushed
the
v7-support-for-request-batching
branch
from
8126f0f
to
56adab1
Compare
reedsa
approved these changes
May 10, 2024
Comment on lines
+65
to
+68
| ) -> Union[ | ||
| Callable[..., Tuple[Tuple[RPCEndpoint, Any], Sequence[Any]]], | ||
| Callable[..., Coroutine[Any, Any, Tuple[Tuple[RPCEndpoint, Any], Sequence[Any]]]], | ||
| ]: |
There was a problem hiding this comment.
Maybe worth creating a type for RequestInformation and use that inside the Callable[...]s. There are a few places these are used. It could end up something like:
Suggested change
| ) -> Union[ | |
| Callable[..., Tuple[Tuple[RPCEndpoint, Any], Sequence[Any]]], | |
| Callable[..., Coroutine[Any, Any, Tuple[Tuple[RPCEndpoint, Any], Sequence[Any]]]], | |
| ]: | |
| ) -> Union[ | |
| Callable[..., RequestInformation], | |
| Callable[..., Coroutine[Any, Any, RequestInformation], | |
| ]: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was wrong?
Closes #832
How was it fixed?
Add batch request support to the
RequestManagervia a context manager. Expose this as a high-level API on theWeb3/AsyncWeb3class itself (w3.batch_requests()).Todo:
EthereumTesterProvidertest caseCute Animal Picture