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

Better Handling of Asyncio #1035

Merged
merged 30 commits into from
Nov 1, 2022
Merged

Conversation

blink1073
Copy link
Contributor

@blink1073 blink1073 commented Oct 20, 2022

  • Drop support for Python 3.7 for 2.x
  • Handle all deprecations relating to event loops with client 8
  • Fork a few of pytest-tornasync fixtures with better handling of event loops
  • Initialize io_loop earlier (cf initialize ioloop earlier #934)
  • Remove utils.run_sync function (was only used in one test).
  • Deprecate redundant utils.run_sync_in_loop, was used by jupyter_server_terminals
  • Get all tests passing

@blink1073 blink1073 marked this pull request as draft October 20, 2022 02:11
@blink1073
Copy link
Contributor Author

Currently working with client 8 and test_gateway.py:

$ pytest test_gateway.py
================================================= test session starts ==================================================
platform darwin -- Python 3.10.7, pytest-7.1.3, pluggy-1.0.0
rootdir: /Users/steve.silvester/workspace/jupyter_server, configfile: pyproject.toml
plugins: anyio-3.6.1, asyncio-0.19.0, timeout-2.1.0, console-scripts-1.3.1
asyncio: mode=auto
timeout: 300.0s
timeout method: signal
timeout func_only: False
collected 23 items

test_gateway.py .......................                                                                          [100%]

================================================= slowest 10 durations =================================================
1.00s call     tests/test_gateway.py::test_channel_queue_get_msg_without_timeout
0.10s call     tests/test_gateway.py::test_channel_queue_get_msg_raises_empty_after_timeout
0.07s setup    tests/test_gateway.py::test_gateway_env_options
0.05s setup    tests/test_gateway.py::test_gateway_session_lifecycle
0.05s setup    tests/test_gateway.py::test_gateway_get_kernelspecs
0.05s setup    tests/test_gateway.py::test_gateway_shutdown[True]
0.05s setup    tests/test_gateway.py::test_gateway_get_named_kernelspec
0.05s setup    tests/test_gateway.py::test_kernel_client_response_router_notifies_channel_queue_when_finished
0.05s setup    tests/test_gateway.py::test_gateway_shutdown[False]
0.05s setup    tests/test_gateway.py::test_gateway_kernel_lifecycle
================================================== 23 passed in 2.14s ==================================================

@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2022

Codecov Report

Attention: Patch coverage is 86.13861% with 14 lines in your changes missing coverage. Please review.

Project coverage is 73.10%. Comparing base (122758f) to head (9c72594).
Report is 317 commits behind head on main.

Files with missing lines Patch % Lines
jupyter_server/pytest_plugin.py 89.58% 2 Missing and 3 partials ⚠️
jupyter_server/serverapp.py 78.57% 1 Missing and 2 partials ⚠️
jupyter_server/base/zmqhandlers.py 60.00% 2 Missing ⚠️
jupyter_server/services/kernels/handlers.py 85.71% 2 Missing ⚠️
jupyter_server/utils.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1035      +/-   ##
==========================================
+ Coverage   72.60%   73.10%   +0.50%     
==========================================
  Files          64       64              
  Lines        8257     8274      +17     
  Branches     1259     1643     +384     
==========================================
+ Hits         5995     6049      +54     
+ Misses       1847     1816      -31     
+ Partials      415      409       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blink1073
Copy link
Contributor Author

Cool, that gets the downstream tests passing again.

]
requires-python = ">=3.7"
requires-python = ">=3.8"
Copy link
Member

Choose a reason for hiding this comment

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

Flagging for public awareness! :)

Copy link
Member

@Zsailer Zsailer Oct 31, 2022

Choose a reason for hiding this comment

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

Thanks for flagging, @vidartf.

@blink1073 you mentioned that SciPy ecosystem has a policy where they support the last few versions, even if the oldest version hasn't reach end-of-life. Do you have a link to something stating this policy? It would be nice to point to it, in case folks have concerns about this bump.

I found this support table from Numpy: https://numpy.org/neps/nep-0029-deprecation_policy.html

This shows that Numpy will drop support for 3.7 in December.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this drop is only for the 2.x branch, we will still keep support in 1.x

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

@blink1073 blink1073 mentioned this pull request Oct 28, 2022
@blink1073 blink1073 marked this pull request as ready for review October 30, 2022 19:50
@blink1073
Copy link
Contributor Author

This is ready for review. I'll follow up with a PR to make the base manager classes async, and deprecate the sync managers.

Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

Thank you, @blink1073. You are the man.

These changes look good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants