Skip to content

Fix threading - #707

Open
jokasimr wants to merge 4 commits into
mainfrom
fix-threading
Open

Fix threading#707
jokasimr wants to merge 4 commits into
mainfrom
fix-threading

Conversation

@jokasimr

Copy link
Copy Markdown
Contributor

Fixes the failing weekly tests on MacOS: #705

@github-actions github-actions Bot added CI essreduce Issues for essreduce. labels Aug 17, 2026
@jokasimr
jokasimr requested a review from nvaytet August 17, 2026 10:01
@jokasimr
jokasimr requested review from MridulS and removed request for nvaytet August 17, 2026 11:17
@SimonHeybrock

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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:

  1. 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.

  2. Instead of silent fallback, raise or warn and encourage the user to install a thread safe backend.

@SimonHeybrock

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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

I mean "can be installed".

@jokasimr

Copy link
Copy Markdown
Contributor Author

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

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

@jokasimr

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI essreduce Issues for essreduce.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants