Fix launch on windows #34

kevin-bates · 2019-11-12T00:46:42Z

Due to event loop implementation differences (see #31), we need to "pin"
the SelectorEventLoop for Windows platforms and use synchronous popen/wait
constructs. Although non-windows platforms will use async constructs,
that decision is maintained in a variable so we can easily change behavior.

@takluyver - I've also added appveyor.yml. It would be great if you could configure appveyor for the repo.

Fixes #31

Due to event loop implementation differences (see #31), we need to "pin" the SelectorEventLoop for Windows platforms and use synchronous popen/wait constructs. Although non-windows platforms will use async constructs, that decision is maintained in a variable so we can easily change behavior. Fixes #31

codecov-io · 2019-11-16T01:22:51Z

Codecov Report

Merging #34 into master will increase coverage by 0.07%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   69.02%   69.09%   +0.07%     
==========================================
  Files          28       28              
  Lines        1995     2074      +79     
==========================================
+ Hits         1377     1433      +56     
- Misses        618      641      +23

Impacted Files	Coverage Δ
jupyter_kernel_mgmt/tests/test_kernelapp.py	`97.05% <100%> (ø)`	⬆️
jupyter_kernel_mgmt/tests/test_localinterfaces.py	`100% <100%> (ø)`	⬆️
jupyter_kernel_mgmt/managerabc.py	`77.27% <100%> (+6.68%)`	⬆️
jupyter_kernel_mgmt/tests/test_async_manager.py	`100% <100%> (ø)`	⬆️
jupyter_kernel_mgmt/tests/test_kernelspec.py	`100% <100%> (+0.81%)`	⬆️
jupyter_kernel_mgmt/tests/test_restarter.py	`100% <100%> (ø)`	⬆️
jupyter_kernel_mgmt/tests/conftest.py	`100% <100%> (ø)`	⬆️
jupyter_kernel_mgmt/tests/test_discovery.py	`98.05% <100%> (+0.03%)`	⬆️
jupyter_kernel_mgmt/util.py	`63.15% <30%> (-36.85%)`	⬇️
jupyter_kernel_mgmt/subproc/manager.py	`68.49% <43.47%> (-11.14%)`	⬇️
... and 13 more

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 90002cd...174f10e. Read the comment docs.

kevin-bates · 2019-11-19T23:38:48Z

Hi @takluyver - could you please review this when you get a chance? I've added appveyor support to the source branch of the PR and the tests are passing on Windows: https://ci.appveyor.com/project/kevin-bates/jupyter-kernel-mgmt

What I can't figure out is why the code coverage stuff is marked as failing? It seems like the coverage may have decreased, at least that's the only thing I see, but I'm surprised that would cause a failure. If that's the case, is there any way to reset the baseline?

I'd really like to get 0.5 out so that the jupyter_server work can more easily move forward. However, my PR in that repo is failing on Windows but I also have to use a temporary modification to pull my dev branch in order to test. (sigh)

I suppose we should configure appveyor for this repo, although that brings up the discussion of moving jupyter_kernel_mgmt (and jupyter_protocol) into the jupyter (or equivalent) org.

takluyver · 2019-11-20T19:57:35Z

jupyter_kernel_mgmt/subproc/manager.py

+            try:
+                self.kernel.wait(sync_wait_timeout)
+            except subprocess.TimeoutExpired:
+                self.log.warning("Timeout expired waiting for process '{}' to terminate.  Continuing...".


This seems very ugly - it's impossible to wait for longer than 2 seconds, but the event loop could be blocked for that long. I think the simplest improvement is to implement a polling loop with something like a 0.1 second async sleep in here - then it works basically as expected, just with a tiny extra delay after the process has really terminated.

Agreed - I'm disappointed I left it like that, but I did - sorry.

It sounds like you're talking about implementing something close to the previous finish_shutdown() method. I'll go ahead with this, leaving things essentially as they are in jupyter_client (i.e., 5 second max).

Similar implementation, but we don't need any timeout here (because you can impose it from outside for async code). Just something like:

while self.kernel.poll() is None: await asyncio.sleep(0.1)

(And no need to apologise - I think I had also thought this was harder to work around before.)

Thanks for the responses. I guess I’m not following how the timeout would be imposed. Could you provide an example?

await asyncio.wait_for(manager.wait(), timeout=3.0)

The asyncio cancellation mechanism should be able to interrupt asyncio.sleep() perfectly fine, but it can't interrupt the blocking Popen.wait() call.

Ok, but manager.wait() is likely only called by manager.kill() so are you suggesting that this be used from kill() and is 3 seconds sufficient?

kill could use something like that - I've no particular preference on what the timeout should be. It's quite unusual for a process to hang so it can't be killed, but it is possible.

But other things could also call wait - e.g. the restarter machinery, or code outside this repo.

jupyter_kernel_mgmt/subproc/manager.py

jupyter_kernel_mgmt/subproc/launcher.py

takluyver · 2019-11-20T20:04:36Z

jupyter_kernel_mgmt/subproc/launcher.py

+        if type(asyncio.get_event_loop_policy()) is WindowsProactorEventLoopPolicy:
+            # WindowsProactorEventLoopPolicy is not compatible with tornado 6
+            # fallback to the pre-3.8 default of Selector
+            asyncio.set_event_loop_policy(WindowsSelectorEventLoopPolicy())


I'm not wild on setting this on import. Generally, I think it should be up to application code, not library code, to set global config like this.

(Application code or test code, of course)

Yeah, I was going to talk to you about this since it's potentially too far down stream in most cases. I'll move it into the util directory as a method - where tests and more focused (i.e., thiner than server) applications can leverage its existence.

takluyver · 2019-11-20T20:06:08Z

appveyor.yml

+      CONDA_PY_SPEC: ">=3.8,<3.9.0a0"
+      CONDA_INSTALL_LOCN: "C:\\Miniconda36-x64"
+    - CONDA_PY: 37
+      CONDA_PY_SPEC: ">=3.7,<3.8.0a0"


I think you can just ask conda for python=3.7, and it knows that means the highest available 3.7 version. This is how I'm used to using it, but maybe that's not recommended, or maybe I'm missing some corner case where specifying it this way makes a difference.

You're probably right, I was following some recent changes in the notebook repo. That said, I can't get the variables to work at all. Very frustrating to "debug" Windows solely by way of appveyor results! 😞
I'll have to play with the settings some more.

takluyver · 2019-11-20T20:11:57Z

I'm not too concerned about the coverage - it shows one failure if the coverage for the new code is lower than the average, and one if the change causes overall coverage to drop. Ignoring it and merging the pull request anyway resets the baseline for future pull requests.

(It would perhaps be nice if this could be represented as less serious than a failing test, but that's how it is)

kevin-bates · 2019-11-22T21:41:33Z

@takluyver - I believe I've addressed the review comments. In addition, I've been able to get all Windows builds passing. The primary issue was that on python 3.8, despite us using the correct event loop, launched ipykernels were not, which prevented their termination, holding onto resources, etc. Once I installed Min's ipykernel patch, all the tests started passing. I think I'd prefer to hold off on merging this until Min's patch is merged, per this comment, but I don't think we need to wait for the next ipykernel release - since this is only comes into play on Windows 3.8 systems.

jupyter_kernel_mgmt/subproc/manager.py

kevin-bates · 2019-11-25T18:27:19Z

@takluyver - all Windows tests are passing as well: https://ci.appveyor.com/project/kevin-bates/jupyter-kernel-mgmt

takluyver · 2019-11-26T10:17:27Z

Thanks Kevin!

kevin-bates · 2019-11-26T15:58:28Z

Thank you for the review and merge @takluyver. I think this is a good time to cut 0.5.0 as it provides a good place to let the jupyter_server pin to this version so that other efforts can take place. Are you okay with that?

kevin-bates requested a review from takluyver November 12, 2019 00:46

Base approach to handle selector policy on NB PR 5047

8117d0c

Adjust some popen params on windows

d4c03c1

takluyver reviewed Nov 20, 2019

View reviewed changes

jupyter_kernel_mgmt/subproc/manager.py Show resolved Hide resolved

takluyver reviewed Nov 20, 2019

View reviewed changes

jupyter_kernel_mgmt/subproc/launcher.py Outdated Show resolved Hide resolved

takluyver reviewed Nov 20, 2019

View reviewed changes

Apply changes per review, fix py38-win issues.

23a2ab6

takluyver reviewed Nov 24, 2019

View reviewed changes

jupyter_kernel_mgmt/subproc/manager.py Outdated Show resolved Hide resolved

takluyver reviewed Nov 24, 2019

View reviewed changes

jupyter_kernel_mgmt/subproc/manager.py Outdated Show resolved Hide resolved

Simply is_alive for Windows, use ipython master for 3.8 fix

174f10e

takluyver merged commit 0eaf82c into takluyver:master Nov 26, 2019

kevin-bates deleted the fix-windows branch November 26, 2019 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix launch on windows #34

Fix launch on windows #34

kevin-bates commented Nov 12, 2019

codecov-io commented Nov 16, 2019 •

edited

Loading

kevin-bates commented Nov 19, 2019

takluyver Nov 20, 2019

kevin-bates Nov 20, 2019

takluyver Nov 21, 2019

takluyver Nov 21, 2019

kevin-bates Nov 21, 2019

takluyver Nov 21, 2019

kevin-bates Nov 21, 2019

takluyver Nov 21, 2019

takluyver Nov 20, 2019

takluyver Nov 20, 2019

kevin-bates Nov 20, 2019

takluyver Nov 20, 2019

kevin-bates Nov 20, 2019

takluyver commented Nov 20, 2019

kevin-bates commented Nov 22, 2019

kevin-bates commented Nov 25, 2019

takluyver commented Nov 26, 2019

kevin-bates commented Nov 26, 2019

Fix launch on windows #34

Fix launch on windows #34

Conversation

kevin-bates commented Nov 12, 2019

codecov-io commented Nov 16, 2019 • edited Loading

Codecov Report

kevin-bates commented Nov 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takluyver commented Nov 20, 2019

kevin-bates commented Nov 22, 2019

kevin-bates commented Nov 25, 2019

takluyver commented Nov 26, 2019

kevin-bates commented Nov 26, 2019

codecov-io commented Nov 16, 2019 •

edited

Loading