-
Notifications
You must be signed in to change notification settings - Fork 308
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
Test with client 8 updates #988
Conversation
Codecov ReportBase: 72.60% // Head: 72.60% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #988 +/- ##
=======================================
Coverage 72.60% 72.60%
=======================================
Files 64 64
Lines 8242 8243 +1
Branches 1378 1378
=======================================
+ Hits 5984 5985 +1
Misses 1844 1844
Partials 414 414
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
This reverts commit c88846c.
* Test with client 8 updates (#988) * pin mistune * fix docs and remove explicit installs
@@ -15,8 +16,9 @@ | |||
except ImportError: | |||
from jupyter_client.jsonutil import date_default as json_default | |||
|
|||
from concurrent.futures import Future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.python.org/3/library/concurrent.futures.html#concurrent.futures.Future says:
Future instances are created by Executor.submit() and should not be created directly except for testing.
Are we not creating such Future
objects in this file now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were already doing that in client for synchronous managers. I really don't see a way around it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pinged Steve about this in a side-bar. We're seeing CI failures in the EG tests when the websocket handling encounters a busy kernel prior to the nudge. It results in the return of a Future
(whose type has changed), ultimately resulting in this log sequence...
[D 2022-09-27 04:19:12.195 EnterpriseGatewayApp] Nudge: not nudging busy kernel 39ce47c4-64c4-4b0e-a2cf-4f998d1b8880
[E 220927 04:19:12 web:1793] Uncaught exception GET /api/kernels/39ce47c4-64c4-4b0e-a2cf-4f998d1b8880/channels (172.17.0.1)
HTTPServerRequest(protocol='http', host='localhost:8888', method='GET', uri='/api/kernels/39ce47c4-64c4-4b0e-a2cf-4f998d1b8880/channels', version='HTTP/1.1', remote_ip='172.17.0.1')
Traceback (most recent call last):
File "/opt/conda/lib/python3.7/site-packages/tornado/websocket.py", line 956, in _accept_connection
await open_result
TypeError: object Future can't be used in 'await' expression
If I revert to jupyter_server < 1.19, CI passes.
Testing against jupyter/jupyter_client#835 (in the prerelease job and as an extra integration test run).