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

Detect cgroups resource limits #3039

Merged
merged 3 commits into from
Sep 10, 2019
Merged

Conversation

jcrist
Copy link
Member

@jcrist jcrist commented Sep 9, 2019

This adds support for detecting resource (CPU and memory) limits set
using cgroups. This makes dask resource detection play nicer with
container systems (e.g. docker), rather than detecting the host memory
and cpus avaialable.

This also centralizes all queries about the host platform to a single
module (distributed.platform), with top-level constants defined for
common usage.

Fixes #1712

This adds support for detecting resource (CPU and memory) limits set
using `cgroups`. This makes dask resource detection play nicer with
container systems (e.g. `docker`), rather than detecting the host memory
and cpus avaialable.

This also centralizes all queries about the host platform to a single
module (`distributed.platform`), with top-level constants defined for
common usage.
Copy link
Member

@mrocklin mrocklin left a comment

Choose a reason for hiding this comment

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

This sounds like a great idea to me!

I don't know enough about cgroups to verify that the choices here are sufficiently generic, but I suspect that @jacobtomlinson or @pentschev may know more.

distributed/platform.py Outdated Show resolved Hide resolved
@pentschev
Copy link
Member

On a very high-level overview, this looks good to me and I also like this idea, so thanks for working on that @jcrist.

One alternative to reading /sys directly is to use https://pypi.org/project/cgroup-utils/, have you checked/thought about that? Not trying to make a case in favor of it, just pointing to its existence, since adding that dependency may also be undesirable.

@pentschev
Copy link
Member

Forgot to mention, but I can take a more detailed look at this PR tomorrow.

@jcrist
Copy link
Member Author

jcrist commented Sep 9, 2019

One alternative to reading /sys directly is to use https://pypi.org/project/cgroup-utils/, have you checked/thought about that?

There's a few different libraries for doing this sort of thing, but all of them seemed to be overkill for the simple queries we need, and there didn't seem to be a strong community standard lib to use. Opted for the simplest thing that works.

distributed/platform.py Outdated Show resolved Hide resolved
@jacobtomlinson
Copy link
Member

This all looks good to me. I don't feel I can add more to the existing reviews right now.

@mrocklin
Copy link
Member

mrocklin commented Sep 10, 2019 via email

@jcrist
Copy link
Member Author

jcrist commented Sep 10, 2019

I believe all comments have been addressed. Should be good-to-go on tests pass.

@mrocklin mrocklin merged commit eecf25b into dask:master Sep 10, 2019
@mrocklin
Copy link
Member

Thanks @jcrist . This is in. I think that this will make folks happy.

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.

Improve memory available detection
6 participants