Set nanny environment variables in config by mrocklin · Pull Request #5098 · dask/distributed

mrocklin · 2021-07-20T23:35:13Z

With the nanny we have the ability to set environment variables in dask workers.
This makes this configurable with the dask.config system.

Somewhat more controversially, this also sets a few common defaults like
MKL_NUM_THREADS, OMP_NUM_THREADS, and MALLOC_TRIM_THRESHOLD

These may have surprising effects, however I suspect that setting
these defaults will help far more people than it would harm.

cc @jacobtomlinson @crusaderky @fjetter and @jcrist who I think are the most likely to be concerned by the default

With the nanny we have the ability to set environment variables in dask workers. This makes this configurable with the dask.config system. Somewhat more controversially, this also sets a few common defaults like MKL_NUM_THREADS, OMP_NUM_THREADS, and MALLOC_TRIM_THRESHOLD These *may* have surprising effects, however I suspect that setting these defaults will help far more people than it would harm.

fjetter

These may have surprising effects, however I suspect that setting
these defaults will help far more people than it would harm.

I'm on board. You're probably right.

What's your reasoning for setting the num threads arguments? That might upset some folks and I don't see as big of a win for them as for the malloc setting.

crusaderky · 2021-07-21T10:09:20Z

    preload: []             # Run custom modules with Nanny
    preload-argv: []        # See https://docs.dask.org/en/latest/setup/custom-startup.html
+    environ:
+      MALLOC_TRIM_THRESHOLD_: 65536


Suggested change

MALLOC_TRIM_THRESHOLD_: 65536

# Linux only; inconsequential on Windows, MacOS, or when using alternative memory managers.

# See https://distributed.dask.org/en/latest/worker.html#memory-not-released-back-to-the-os

MALLOC_TRIM_THRESHOLD_: 65536

Historically we've avoided putting lots of comments in the main config file, relying instead on the description in the schema file (which gets put into docs). I'm also OK being silent about this because it doesn't have any negative effect.

What's the reasoning behind this value for MALLOC_TRIM_THRESHOLD_?

crusaderky · 2021-07-21T10:10:43Z


        self.Worker = Worker if worker_class is None else worker_class
-        self.env = env or {}
+        self.env = env or dask.config.get("distributed.nanny.environ")


Suggested change

self.env = env or dask.config.get("distributed.nanny.environ")

self.env = dask.config.get("distributed.nanny.environ")

self.env.update(env or {})

Also I could not find anything that passes the env parameter to the Nanny constructor. Maybe we could deprecate it? Unless some dask users actually create Nanny objects by hand?

I'm inclined to leave the deprecation question to other work if that's ok. It seems harmless to leave here for now and there are other things that seem higher priority.

Co-authored-by: crusaderky <crusaderky@gmail.com>

jrbourbeau

I'd like to hold off on merging this until after the release this Friday. That will give us a bit more time to hear from folks using the main branch of distributed

jrbourbeau · 2021-07-21T15:39:33Z

    preload: []             # Run custom modules with Nanny
    preload-argv: []        # See https://docs.dask.org/en/latest/setup/custom-startup.html
+    environ:
+      MALLOC_TRIM_THRESHOLD_: 65536


What's the reasoning behind this value for MALLOC_TRIM_THRESHOLD_?

mrocklin · 2021-07-21T15:52:04Z

What's the reasoning behind this value for MALLOC_TRIM_THRESHOLD_?

@crusaderky suggested it, and I trust @crusaderky :)

mrocklin · 2021-07-21T15:54:06Z

What's your reasoning for setting the num threads arguments? That might upset some folks and I don't see as big of a win for them as for the malloc setting.

Anyone using blas/lapack today essentially needs to set these in order to avoid massively oversubscribing their machines. OpenBLAS in particular will often just shut down the program if you try to use more than twice the number of threads on the system.

jacobtomlinson

This generally sounds good to me.

My only concern is that this feels a little magic and I think it could be easy for a user to not understand where an environment variable is coming from. It would be easy to set something in your YAML and forget about it.

For example if I set:

distributed:
  nanny:
    environ:
      FOO: "bar"

and then run

$FOO=baz dask-worker tcp://localhost:8786

The environment variable on the worker will be bar. This doesn't feel intuitive.

crusaderky · 2021-07-22T14:01:59Z

What's the reasoning behind this value for MALLOC_TRIM_THRESHOLD_?

@crusaderky suggested it, and I trust @crusaderky :)

0 or 65536 is pretty much the same for CPython and CPython extenions like numpy, because PyMalloc invokes the underlying malloc() in large-ish chunks.
However, C/C++ libraries that invoke malloc directly - which are the whole reason why the glibc implements its own user-space memory manager - would risk getting severely slowed down by setting the trim to 0.

mrocklin · 2021-07-26T18:49:24Z

@jrbourbeau are we ok to merge this in?

jrbourbeau

Thanks @mrocklin. It looks like there's no objections to setting these default environment variables. Will merge once CI finishes

@jacobtomlinson, to your point about environment variable priority, Matt included an update where environment variables in os.environ will take priority over those in the distributed.nanny.environ configuration setting. I think this addresses the concerns you raised, but if not I'm happy to follow up in a separate PR

In #5098 we set a malloc trim threshold by default to more aggressively control memory trimming. also related #7177 At the same time, we included these default settings but didn't have incredibly solid arguments for it. It's been a long standing best practice when using dask to disable this nested parallelism. We haven't received a lot of user feedback about this. However, we had some internal reports of users who were struggling with this because this was quite unexpected behavior for them and non-trivial to debug for the ordinary end user. In apache/arrow#38389 (comment) this also suggests to negatively impact read performance of parquet tables. We should consider removing this again

fjetter approved these changes Jul 21, 2021

View reviewed changes

crusaderky reviewed Jul 21, 2021

View reviewed changes

Comment thread distributed/tests/test_nanny.py Outdated

mrocklin and others added 2 commits July 21, 2021 06:50

Update distributed/tests/test_nanny.py

b6524ab

Co-authored-by: crusaderky <crusaderky@gmail.com>

overlap user provided env on top of config

5d7c417

jrbourbeau reviewed Jul 21, 2021

View reviewed changes

jacobtomlinson reviewed Jul 22, 2021

View reviewed changes

Comment thread distributed/nanny.py Outdated

respect local environment over config

62d3805

mrocklin force-pushed the nanny-environ-config branch from a886c06 to 62d3805 Compare July 22, 2021 14:19

Only temporarily patch environment variable in test

2b77038

jrbourbeau approved these changes Jul 26, 2021

View reviewed changes

jrbourbeau merged commit b37ac9d into dask:main Jul 26, 2021

jrbourbeau mentioned this pull request Jul 29, 2021

Document automatic MALLOC_TRIM_THRESHOLD_ environment variable #5139

Merged

fjetter mentioned this pull request Aug 9, 2021

scheduler immediately takes up > 40 GB of memory #5180

Closed

fjetter mentioned this pull request Oct 23, 2023

[RFC] Do not set parallelism controlling env vars #8297

Closed

2 tasks

dchudz mentioned this pull request Sep 29, 2025

remove *NUM_THREADS env variables from default config #9081

Open

2 tasks

	self.env = env or dask.config.get("distributed.nanny.environ")
	self.env = dask.config.get("distributed.nanny.environ")
	self.env.update(env or {})

Uh oh!

Uh oh!

Conversation

mrocklin commented Jul 20, 2021

Uh oh!

fjetter left a comment

Choose a reason for hiding this comment

Uh oh!

crusaderky Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

mrocklin Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

jrbourbeau Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

crusaderky Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

mrocklin Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

jrbourbeau Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

mrocklin commented Jul 21, 2021

Uh oh!

mrocklin commented Jul 21, 2021

Uh oh!

jacobtomlinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crusaderky commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrocklin commented Jul 26, 2021

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

crusaderky commented Jul 22, 2021 •

edited

Loading