ipyparallel downstream test broken

[blink1073]

It looks like #869 broke our downstream ipyparallel tests. I hadn't been able to diagnose it when reviewing the PR because the job wasn't showing any output at all.

[blink1073]

cc @Carreau @minrk

[Carreau]

I think the failing test is ipyparallel/tests/test_client.py::TestClient::test_shutdown, my guess is that killpg kills pytest itself while testing.

[minrk]

Yeah, calling killpg on yourself is absolutely not a safe way to only kill subprocesses (or even interrupt them). For one, it kills the current process, not just children. That means it's likely preventing clean shutdown as well (especially _killpg(SIGKILL)). But it's also true that it will kill any parents if they launch kernels without starting a new process group (as ipyparallel does). So I think it's better to do nothing than to use killpg there, if psutil is unavailable.

I also think our _killpg needs to check if pgid == os.getpid(), because any time that's not true, it's going to do the wrong thing.

[Carreau]

So basically I should not reply on killpg and have a hard dependency on psutil.

[minrk]

Yeah, I think that's the way to go. The big difference is the psutil implementation reliably signals all children, while the pgid at least includes the current process itself (I think this is intentional for interrupt_request, but not shutdown), and possibly several generations up (though probably not if the kernel was started by jupyter-client, which forks a new process group for each kernel by default).

I can have a go, if you like. I'm looking at it now.

[Carreau]

I can have a go, if you like. I'm looking at it now.

sure, if it takes you 30 minutes – you are more familiar with subprocesses than I – if more I can take care of it.

[minrk]

Ah, one more snag: need to check pgid of children, because the current psutil implementation will kill all children, even if they have started a new process group. This would be the wrong thing to do if, for instance, someone spawned a process that should stick around after the kernel restarts (this happens in typical use of ipyparallel, too).

An example process tree:

• parent, pgid1
  • kernel, pgid1
    • child1, pgid1
    • child2, pgid2

We really only want to signal child1 in this case, but the killpg will get parent and kernel, while psutil.children will catch both children if we don't also check pgid.

sure, if it takes you 30 minutes

Sure, I'll have a go. I happened to do a lot of similar stuff with the psutil rewrite in ipyparallel over the summer, so it's pretty fresh for me.

[Carreau]

Ok, I'll trust you with that. I know that some of the requests were for dask cluster that did not stop after restarting kernels, so I hope we wont end up with folks asking to have a --really-kill-everything flag.

[minrk]

I know that some of the requests were for dask cluster that did not stop after restarting kernels, so I hope we wont end up with folks asking to have a --really-kill-everything flag.

that's ~exactly what IPython Parallel does, and I'd argue it's the responsibility of the dask cluster APIs to take care of it. If they don't fork the pgid, it'll happen automatically, but if they do, it's up to them to clean up. They can further use parent pollers to tell the cluster to shutdown when their parents go away. We do both in ipyparallel.

[minrk]

#874 should work, I think.