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

Upgrade anyio to v3 #492

Merged
merged 8 commits into from
May 3, 2021
Merged

Conversation

mwakaba2
Copy link
Member

@mwakaba2 mwakaba2 commented Apr 24, 2021

This fixes #487 (Anyio version 3 breaks AsyncContentsManager).

Changes

  • Upgrade anyio from v2 to v3.
  • Replace deprecated function run_sync_in_worker_thread with to_thread.run_sync.
  • Tests: Prefix contents manager fixture with jp

How to test

$  pip install -e ".[test]"
$  pip install git+https://github.com/agronholm/anyio.git@fix-264
$  python -m pytest -v jupyter_server/tests/services/contents/

This PR depends on Anyio's Refactored asyncio thread pooling .

Todo

setup.cfg Outdated Show resolved Hide resolved
@kevin-bates
Copy link
Member

Excellent, thank you @mwakaba2 and @agronholm! I'm going to place this into DRAFT for now until Alex's anyio PR is merged and released.

@kevin-bates kevin-bates marked this pull request as draft April 26, 2021 15:23
Carreau and others added 6 commits April 30, 2021 20:43
ipython_genutils is complicated to package on some linux distro because
of nose, and in general not useful as it mostly offered python 2/3
compat layer.

Goal always has been to remove any usage of ipython_genutils
This uses an autoreformatter to reformat and fix some of the common
mistakes in numpydoc.

Some of this is purely visual, but other like space before colon in
parameters actually have semantic values. In the case of space before
colon this is to make sure that nupmydoc properly distinguish the
parameter name from its type.
This change also prevents permission-related exceptions from logging the file.

Co-authored-by: Shane Canon <scanon@gmail.com>
Co-authored-by: Thomas Kluyver <thomas@kluyver.me.uk>
@mwakaba2 mwakaba2 force-pushed the upgrade-anyio-3 branch 2 times, most recently from 280c3da to 409b5b2 Compare May 1, 2021 02:54
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2021

Codecov Report

Merging #492 (eff9b0a) into master (ad9ccb3) will decrease coverage by 0.03%.
The diff coverage is 93.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
- Coverage   77.74%   77.71%   -0.04%     
==========================================
  Files         106      106              
  Lines        9276     9288      +12     
  Branches     1001     1001              
==========================================
+ Hits         7212     7218       +6     
- Misses       1707     1711       +4     
- Partials      357      359       +2     
Impacted Files Coverage Δ
jupyter_server/services/contents/filemanager.py 66.52% <76.92%> (+0.21%) ⬆️
jupyter_server/services/contents/fileio.py 75.64% <90.90%> (+0.38%) ⬆️
...upyter_server/services/contents/filecheckpoints.py 63.58% <100.00%> (+0.64%) ⬆️
...pyter_server/services/contents/largefilemanager.py 82.97% <100.00%> (+0.56%) ⬆️
...ter_server/tests/services/contents/test_manager.py 97.28% <100.00%> (ø)
jupyter_server/services/kernels/handlers.py 61.37% <0.00%> (-1.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad9ccb3...eff9b0a. Read the comment docs.

@mwakaba2 mwakaba2 changed the title [WIP] Upgrade anyio to v3 Upgrade anyio to v3 May 1, 2021
@mwakaba2 mwakaba2 changed the title Upgrade anyio to v3 [WIP] Upgrade anyio to v3 May 1, 2021
@kevin-bates
Copy link
Member

Good to see the quick turnaround on the anyio patch, although it looks like 3.6 still has some heartburn.

Choices...

  1. Cap anyio < 3 for python < 3.7 and alias run_sync to run_sync_in_worker_thread on 3.6?
  2. Disable AsyncContentsManager on 3.6 given it is EOL at EOY and probably has few consumers at this point?
  3. Try to determine what it is about 3.6 that's causing the hang?
  4. ?

The PR for #490 will probably need option 1.

@yan12125
Copy link

yan12125 commented May 2, 2021

Disable AsyncContentsManager on 3.6 given it is EOL at EOY and probably has few consumers at this point?

FWIW, according to https://pypistats.org/packages/jupyter-server, Python 3.6 downloads account for 13%~36% on PyPI for the past 30 days.

@kevin-bates
Copy link
Member

Thanks for the metric @yan12125 - that's good to know, although I suspect the percentage overriding the default contents manager to the async version is nearly zero. Nevertheless, I think the first option would be the best step forward (barring a more graceful solution) as it can also be used to accomplish #490.

@mwakaba2
Copy link
Member Author

mwakaba2 commented May 3, 2021

Yeah I think the first option sounds good to unblock #490. I'll also look into the issue with python 3.6 and check with anyio if it's related to their new update.

@mwakaba2 mwakaba2 changed the title [WIP] Upgrade anyio to v3 Upgrade anyio to v3 May 3, 2021
@mwakaba2 mwakaba2 marked this pull request as ready for review May 3, 2021 03:05
@agronholm
Copy link

If the problem with py3.6 is caused by AnyIO, I would like to know.

@mwakaba2
Copy link
Member Author

mwakaba2 commented May 3, 2021

@kevin-bates I'd appreciate your review on this PR.

@agronholm I created a new issue for the py3.6 error. #505 It looks like it's an anyio issue.
I'd appreciate if you can take a look. I'll be able to debug further after this Wednesday.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

These changes look good and allow us to proceed with #490. Thank you!

@mwakaba2
Copy link
Member Author

mwakaba2 commented May 3, 2021

thank you @kevin-bates ! I don't have permission to merge the pull request, so if it all looks good, please feel free to merge it.

@kevin-bates
Copy link
Member

Thanks Mariko - I was giving others a chance to comment, but these seem straightforward enough - so I'll go ahead.

@kevin-bates kevin-bates merged commit 678878f into jupyter-server:master May 3, 2021
@Zsailer
Copy link
Member

Zsailer commented May 3, 2021

@mwakaba2, I've upgraded your permissions to "write" 😎 . My apologies for not doing this sooner!

@mwakaba2
Copy link
Member Author

mwakaba2 commented May 3, 2021

sweet! thanks so much. I look forward to contributing more! 😎

@mwakaba2 mwakaba2 deleted the upgrade-anyio-3 branch May 3, 2021 22:47
hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023
luong-komorebi added a commit to luong-komorebi/enterprise_gateway that referenced this pull request Sep 24, 2023
error when running unittest:
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/assertion/rewrite.py:178: in exec_module
    exec(co, module.__dict__)
enterprise_gateway/tests/test_gatewayapp.py:11: in <module>
    from enterprise_gateway.enterprisegatewayapp import EnterpriseGatewayApp
enterprise_gateway/enterprisegatewayapp.py:19: in <module>
    from jupyter_server.serverapp import random_ports
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/jupyter_server/serverapp.py:77: in <module>
    from .services.contents.filemanager import AsyncFileContentsManager, FileContentsManager
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/jupyter_server/services/contents/filemanager.py:15: in <module>
    from anyio import run_sync_in_worker_thread
E   ImportError: cannot import name 'run_sync_in_worker_thread' from 'anyio' (/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/anyio/__init__.py)

ref:
jupyter-server/jupyter_server@678878f
jupyter-server/jupyter_server#492
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.

Anyio version 3 breaks AsyncContentsManager
8 participants