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

Vendor DataLoader from aiodataloader and move get_event_loop() out of __init__ function. #1459

Merged
merged 5 commits into from
Sep 7, 2022
Merged

Vendor DataLoader from aiodataloader and move get_event_loop() out of __init__ function. #1459

merged 5 commits into from
Sep 7, 2022

Conversation

flipbit03
Copy link
Contributor

DataLoaders are trying to get the asyncio's event loop too early (on __init__) and this is generating problems like making pytest + async unusable. Since there is no event loop at that moment, the first DataLoader instantiated is actually creating a new event loop. This generates the infamous "Future tied to a different loop" problem many people are facing.

In this PR, we are:

  • Vendoring aiodataloader's DataLoader class into the utils/ module
  • Adding a fix that moves get_event_loop out of the class' initializer and into a property which only gets called when actually needed, offsetting the chicken-and-egg problem we are dealing with.

The base for this work is dataloader.py, which is MIT Licensed code from Syrus Akbary

@codecov
Copy link

codecov bot commented Sep 5, 2022

Codecov Report

Merging #1459 (1e7bfb7) into master (45986b1) will increase coverage by 0.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1459      +/-   ##
==========================================
+ Coverage   95.44%   95.72%   +0.27%     
==========================================
  Files          49       50       +1     
  Lines        1582     1683     +101     
==========================================
+ Hits         1510     1611     +101     
  Misses         72       72              
Impacted Files Coverage Δ
graphene/utils/dataloader.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@flipbit03 flipbit03 changed the title Vendor DataLoader from aiodataloader and move get_event_loop() out of __init__ function. Vendor DataLoader from aiodataloader and move get_event_loop() out of __init__ function. Sep 5, 2022
@@ -2,6 +2,10 @@
exclude = setup.py,docs/*,*/examples/*,graphene/pyutils/*,tests
max-line-length = 120

# This is a specific ignore for Black+Flake8
# source: https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html#id1
extend-ignore = E203
Copy link
Contributor Author

Choose a reason for hiding this comment

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

without this line, flake8 was constantly clashing with black over array slice notation whitespacing rules.

@erikwrede
Copy link
Member

Seems like test_dataloader is never run, explaining the decrease in coverage

flipbit03 and others added 5 commits September 6, 2022 11:33
…avior from `__init__` to a property which only gets resolved when actually needed (this will solve PyTest-related early get_event_loop() issues)
…ility to pass in a custom event loop, if needed.
@erikwrede erikwrede merged commit 694c1db into graphql-python:master Sep 7, 2022
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.

2 participants