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

Only kill children in process group at shutdown #874

Merged
merged 3 commits into from Feb 21, 2022

Conversation

minrk
Copy link
Member

@minrk minrk commented Feb 21, 2022

  • not the whole process group (killpg), which includes kernel itself and possibly parents (closes ipyparallel downstream test broken #872)
  • not children which have started their own process groups, which is the standard indicator for "leave me alone when my parent shuts down"

closes #873

- not the whole process group, which includes self and possibly parents
- not children which have started their own process groups
@minrk
Copy link
Member Author

minrk commented Feb 21, 2022

One question here: this cleanup logic depends on psutil, but I haven't made psutil a hard dependency. So we have a choice:

  1. make this cleanup an optional enhancement when psutil is available, or
  2. bump psutil to a hard dependency so this always runs

@Carreau
Copy link
Member

Carreau commented Feb 21, 2022

Previous PR made is a dependency on windows, so I think we can make it a hard dep.

now needed for cleanup on all platforms,
as well as usage_requests
@minrk
Copy link
Member Author

minrk commented Feb 21, 2022

Now requires psutil, removing conditions for handling its absence

Moving _is_debugpy_available to .debugger to avoid having import that
much of the package on wheel building. In particular this was forcing
`psutil`  to be importable to build this package
@Carreau
Copy link
Member

Carreau commented Feb 21, 2022

pushed 5c16fde to not make psutil necessary to build the wheel and changing what is imported where.

@echarles Is that ok with you ?

@echarles
Copy link
Member

@echarles Is that ok with you ?

Moving _is_debugpy_available to debugger.py makes sense (related to debugpy dependency). Looking more to the psutil dependency, main branch only installs psutil for windows.

'psutil;platform_system=="Windows"',

which is needed on all platform to honor the usage_request

async def usage_request(self, stream, ident, parent):
reply_content = {
'hostname': socket.gethostname()
}
if psutil is None:
return reply_content

Any particular reason to restrict psutil to only windows?

@Carreau
Copy link
Member

Carreau commented Feb 21, 2022

Any particular reason to restrict psutil to only windows?

Well originally it was because killpg is not avail on windows, I guess it was not added as a dep when usage_request was added, so it's an argument to make it a hard dependency.

@Carreau
Copy link
Member

Carreau commented Feb 21, 2022

Ok, It seem we are all in agreement, merging.

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

Successfully merging this pull request may close these issues.

ipyparallel downstream test broken
4 participants