Fix threading by jokasimr · Pull Request #707 · scipp/ess

jokasimr · 2026-08-17T09:59:22Z

Fixes the failing weekly tests on MacOS: #705

SimonHeybrock · 2026-08-17T11:42:56Z

Can you clarify what the implications of this are (performance?), and whether we fallback in practice (production) — and if so, how we can avoid that. It seems like this might silently introduce performance regressions, depending on environment.

jokasimr · 2026-08-17T11:56:36Z

Can you clarify what the implications of this are (performance?), and whether we fallback in practice (production) — and if so, how we can avoid that. It seems like this might silently introduce performance regressions, depending on environment.

The implications are that in environments that don't have tbb or openmp we will not use the Numba interpolation implementation.

This will lead to a regression in environments without those libraries, because those will fall back to the slower scipy implementation. But that is almost certainly better than randomly crashing, which is what might happen otherwise.

The required libraries should be available on all our production platforms. It might of course fall back in some user environment that we don't control.

Alternatives:

Don't use dask threading. This will make this issue less likely to happen in our typical setups, but we loose dask thread parallelism and the workflows will still be thread-unsafe and anyone running the workflows in a thread pool might experience the same issue.
Instead of silent fallback, raise or warn and encourage the user to install a thread safe backend.

SimonHeybrock · 2026-08-17T13:23:19Z

Have you looked into our actual envs — when you say "should be available", does that mean can be installed or are installed? What environments do not have tbb or openmp? Disabling threaded dask for tests might be a less-invasive fix for CI, if we need more time to figure that out.

jokasimr · 2026-08-17T13:27:09Z

when you say "should be available", does that mean can be installed or are installed?

I mean "can be installed".

jokasimr · 2026-08-17T13:30:44Z

Disabling threaded dask for tests might be a less-invasive fix for CI

Maybe, but then we don't catch issues like this.

jokasimr · 2026-08-17T13:40:44Z

I don't really think this is a big change. The code already falls back to the non-numba implementation if numba is not installed, and numba is not an explicit requirement of essreduce. This PR changes that condition to: "fall back to the non-numba implementation if numba with a threadsafe backend is not installed".

SimonHeybrock · 2026-08-17T13:52:11Z

I don't really think this is a big change. The code already falls back to the non-numba implementation if numba is not installed, and numba is not an explicit requirement of essreduce.

Hmm, does seems odd. Isn't it what the wavelength-LUT code wants to rely on @nvaytet? Should it be added, for essreduce, or the technique packages? Do our VISA images have it?

This PR changes that condition to: "fall back to the non-numba implementation if numba with a threadsafe backend is not installed".

Knowing "I have to install numba to get the fast implementation" is relatively easy, knowing that there might be a silent fallback to a slow implementation if some threading library (the user may have never heard of) is not available is arguably worse.

jokasimr · 2026-08-18T05:50:59Z

Knowing "I have to install numba to get the fast implementation" is relatively easy, knowing that there might be a silent fallback to a slow implementation if some threading library (the user may have never heard of) is not available is arguably worse.

Right now the user still has to remember to install some threading library, or they might see random crashes.

SimonHeybrock · 2026-08-18T06:05:10Z

Knowing "I have to install numba to get the fast implementation" is relatively easy, knowing that there might be a silent fallback to a slow implementation if some threading library (the user may have never heard of) is not available is arguably worse.

Right now the user still has to remember to install some threading library, or they might see random crashes.

In other words, it is not silent, that is my point. I think not silently degrading performance is better.

jokasimr · 2026-08-19T13:37:46Z

I've now added a workflow parameter for selecting the wavelength unwrapping backend.

If the value of that parameter is numba (the default) then it will check if numba is installed with a thread safe backend, if it isn't then it falls back to using scipy (preserving current behavior), but it will also raise a deprecation warning explaining the issue. That lets us change this to be a hard error in the future.

SimonHeybrock

Looks like a reasonable approach, what do you think @nvaytet or @jl-wynen ?

jl-wynen · 2026-08-20T11:21:06Z

+    try:
+        from numba import config, get_num_threads, threading_layer
+
+        config.THREADING_LAYER = 'threadsafe'


I don't like that this sets a global variable. It will be hard to debug threading behaviour in code that uses the workflow.
Is this strictly required here?

Instead of modifying THREADING_LAYER we could just check that it is one of the threadsafe ones.
I think numba will by default select a threadsafe layer if there is one available.

jl-wynen · 2026-08-20T11:23:55Z

+
+    def interpolate_after_barrier(_):
+        barrier.wait()
+        return numba_interp(times, distances)


I think you need a second barrier after the call to numba_interp. Otherwise, one thread can call the interpolator and finish before the other calls the interpolator.

But in general, there is no guarantee that both threads will be in the interpolator at the same time.

I don't think the second barrier will help with that because, like you note, the threads will not be in the interpolator at the same time, and I think that is required for the crash to occur.

There's no guarantee the threads will be in the interpolator at the same time, but I think it is quite likely, we could increase the number of points to make it more likely.

We could also just remove the test.

jl-wynen · 2026-08-20T11:31:58Z

 python = "3.11.*"

+[target.osx-arm64.dependencies]
+llvm-openmp = "*"


Why is this required? And how does this map onto the wheel and conda package dependencies?

It is required because the osx-arm64 platform does not have tbb or openmp available by default, and then numba will use a non-thread safe backend, and the tests in essreduce will fall back to using the scipy interpolator.

Given that most of ESS is using Macs, should we add this to our package requirements?

But this only affects dev environments. The package deps are in pyproject.toml.

pyproject.toml cannot install a conda package, so as I understand it we can not make this part of the dependencies of essreduce?

However, we can make sure to add this to the ESS environment that is deployed on VISA and that users can download to reproduce the environment locally.

@MridulS What do you think? We'd also need to make sure this is installed in the ESSlivedata backends for example, which just use pip.

This shouldn't have any affect on visa or livedata as they are anyway running on linux boxes, but I'm not a 100% sure if this is going in the right direction. Something seems off, where is windows/linux getting tbb/openmp from then? Discussing this with Johannes.

I think the cleaner solution here for now is to add this to our pixi.toml

[feature.essreduce.dependencies] numba = ">=0.63" # Prefer conda-forge Numba as it installs thread safe backends for all platforms

pixi solves the conda packages first, so this numba should take precedence over the one defined by the test optional deps in essreduce pyproject.toml

MridulS · 2026-08-20T13:34:45Z

https://github.com/scipp/ess/blob/main/pixi.lock does have _openmp_mutex for win64, and linux-64. Not for macos ....

MridulS · 2026-08-20T19:18:45Z

After some digging found this numba/numba#10492, looks like putting numba as a dependency of essreduce test extras (which forces to pull numba in from pypi) messes this up. The conda-forge numba correctly installs the threading backends.

github-actions Bot added CI essreduce Issues for essreduce. labels Aug 17, 2026

jokasimr requested review from MridulS and nvaytet and removed request for nvaytet August 17, 2026 10:01

jl-wynen mentioned this pull request Aug 20, 2026

Drop Python 3.11, declare and test up to Python 3.14 #686

Merged

SimonHeybrock reviewed Aug 20, 2026

View reviewed changes

jl-wynen reviewed Aug 20, 2026

View reviewed changes

jokasimr force-pushed the fix-threading branch from 3dd99e7 to 6ed2221 Compare August 20, 2026 12:28

jokasimr added 3 commits August 21, 2026 09:37

feat: add selectable frame-unwrapping backend

291a5d3

ci: prefer conda-forge Numba for essreduce

9449cdb

ci: update pixi lock

9a8ceb4

jokasimr force-pushed the fix-threading branch from 8b762f8 to 9a8ceb4 Compare August 21, 2026 07:39

Uh oh!

Conversation

jokasimr commented Aug 17, 2026

Uh oh!

SimonHeybrock commented Aug 17, 2026

Uh oh!

jokasimr commented Aug 17, 2026

Uh oh!

SimonHeybrock commented Aug 17, 2026

Uh oh!

jokasimr commented Aug 17, 2026

Uh oh!

jokasimr commented Aug 17, 2026

Uh oh!

jokasimr commented Aug 17, 2026

Uh oh!

SimonHeybrock commented Aug 17, 2026

Uh oh!

jokasimr commented Aug 18, 2026

Uh oh!

SimonHeybrock commented Aug 18, 2026

Uh oh!

jokasimr commented Aug 19, 2026

Uh oh!

SimonHeybrock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jokasimr Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MridulS Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MridulS commented Aug 20, 2026

Uh oh!

MridulS commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jokasimr Aug 20, 2026 •

edited

Loading

MridulS Aug 20, 2026 •

edited

Loading