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

Update asyncio/Tornado #4510

Closed
wants to merge 4 commits into from
Closed

Update asyncio/Tornado #4510

wants to merge 4 commits into from

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Mar 7, 2023

make_current deprecation was reverted in Python and will not give a warning in Tornado 6.3 and forward (tornadoweb/tornado#3216).

The change in conftest.py is to avoid this warning, though I'm unsure if it is the right way.

  /home/shh/miniconda3/envs/holoviz/lib/python3.10/site-packages/tornado/ioloop.py:265: DeprecationWarning: There is no current event loop
    loop = asyncio.get_event_loop()

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #4510 (47b1141) into main (c9702ee) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4510      +/-   ##
==========================================
+ Coverage   82.32%   82.35%   +0.02%     
==========================================
  Files         243      243              
  Lines       35579    35585       +6     
==========================================
+ Hits        29292    29305      +13     
+ Misses       6287     6280       -7     
Flag Coverage Δ
ui-tests 35.88% <100.00%> (+0.02%) ⬆️
unitexamples-tests 74.27% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
panel/io/server.py 74.86% <100.00%> (ø)
panel/tests/conftest.py 89.62% <100.00%> (+0.26%) ⬆️
panel/io/state.py 70.56% <0.00%> (+0.17%) ⬆️
panel/reactive.py 80.72% <0.00%> (+0.37%) ⬆️
panel/io/reload.py 73.91% <0.00%> (+2.17%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@philippjfr
Copy link
Member

So I'm not 100% convinced we should actually make this change. Each thread should get its own event loop so it makes sense to me that we should use make_current=False in the IOLoop constructor and then have the thread start the event loop. Then again my mental model of this may not be 100% correct.

@philippjfr
Copy link
Member

For the time being I'm going to keep the current approach. It seems more correct to me for the thread to start the event loop.

@philippjfr philippjfr closed this Mar 14, 2023
@hoxbro hoxbro deleted the update_asyncio_change branch March 14, 2023 19:56
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