Skip to content

fix: detect any non-main thread, not just ThreadPoolExecutor by name - #212

Merged
flying-sheep merged 1 commit into
scverse:mainfrom
selmanozleyen:refactor/no-thread-string-matching
Sep 18, 2026
Merged

flying-sheep merged 1 commit into
scverse:mainfrom
selmanozleyen:refactor/no-thread-string-matching

Conversation

@selmanozleyen

Copy link
Copy Markdown
Member

The fact that we check by string matching "ThreadPoolExecutor" seems brittle to me. Even if it's the only option we should've document it as an assumption because in cases like ThreadPoolExecutor(thread_name_prefix=...) this fails. And probably also in joblib it will fail.

Instead why don't we check if the caller is inside the main thread or not? I checked with jupyter and the executor there is in main thread as well.

@codecov

codecov Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.32%. Comparing base (4fa33a0) to head (5a1f5da).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
- Coverage   99.33%   99.32%   -0.01%     
==========================================
  Files          21       21              
  Lines         598      597       -1     
==========================================
- Hits          594      593       -1     
  Misses          4        4              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@selmanozleyen
selmanozleyen force-pushed the refactor/no-thread-string-matching branch 2 times, most recently from 03318a4 to 5a1f5da Compare September 17, 2026 18:15
A thread name is not a contract: ThreadPoolExecutor(thread_name_prefix=...) renames its
workers, and joblib, dask and hand-started threads never matched at all. Identity against
threading.main_thread() is public API and covers all of them.

The check is renamed because its subject changed. It no longer asks whether this is a pool
worker, it asks whether this is the thread the interpreter started on.
@codspeed

codspeed Bot commented Sep 17, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 11.23%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 2 improved benchmarks
✅ 230 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
test_to_dense_benchmark[scipy.sparse.csc_array-float64-C] 83.1 ms 74.4 ms +11.76%
test_to_dense_benchmark[scipy.sparse.csr_matrix-float32-F] 77.2 ms 69.7 ms +10.7%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing selmanozleyen:refactor/no-thread-string-matching (5a1f5da) with main (4fa33a0)

Open in CodSpeed

@flying-sheep

Copy link
Copy Markdown
Member

Great, thank you!

I think the only reason we don’t do that is that I lifted that implementation from somewhere else and didn’t know that there’s a better way.

@flying-sheep flying-sheep added the run-gpu-ci Apply this label to run GPU CI once label Sep 18, 2026
@flying-sheep flying-sheep changed the title refactor: detect any non-main thread, not just ThreadPoolExecutor by name fix: detect any non-main thread, not just ThreadPoolExecutor by name Sep 18, 2026
@flying-sheep
flying-sheep merged commit 57365b8 into scverse:main Sep 18, 2026
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-gpu-ci Apply this label to run GPU CI once

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants