Skip to content

Limit threads for mris_expand - #491

Open
chrisadamsonmcri wants to merge 2 commits into
nipreps:masterfrom
chrisadamsonmcri:master
Open

Limit threads for mris_expand#491
chrisadamsonmcri wants to merge 2 commits into
nipreps:masterfrom
chrisadamsonmcri:master

Conversation

@chrisadamsonmcri

@chrisadamsonmcri chrisadamsonmcri commented Sep 12, 2025

Copy link
Copy Markdown

Limits the threads for mris_expand and changes the OpenMP scheduler to dynamic.

Fixes #490.

@codecov

codecov Bot commented Sep 12, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.03%. Comparing base (afde296) to head (5e0b15a).
⚠️ Report is 131 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #491      +/-   ##
==========================================
+ Coverage   70.87%   75.03%   +4.16%     
==========================================
  Files          25       25              
  Lines        2067     2067              
  Branches      268      268              
==========================================
+ Hits         1465     1551      +86     
+ Misses        544      448      -96     
- Partials       58       68      +10     
Flag Coverage Δ
ds054 44.95% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@effigies effigies closed this Sep 12, 2025
@effigies

Copy link
Copy Markdown
Member

FWIW, I would delete that _num_threads_update method entirely and update

midthickness = pe.MapNode(
MakeMidthickness(thickness=True, distance=0.5, out_name='midthickness'),
iterfield='in_file',
name='midthickness',
n_procs=min(omp_nthreads, 12),
)

to use:

     midthickness = pe.MapNode(
         MakeMidthickness(
             thickness=True,
             distance=0.5,
             out_name='midthickness',
+            environ={'OMP_SCHEDULE': 'dynamic'},
         ),
         iterfield='in_file',
         name='midthickness',
-        n_procs=min(omp_nthreads, 12),
+        n_procs=min(omp_nthreads, 8),
    )

The purpose of the _num_threads_update() was to allow the job to claim,e.g, 8 threads while actually allocating 12. If we aren't doing a trick like that, it's cleaner to put the min/max logic where the node is created. And any Nipype CommandLine interface can take an environ argument, so let's do that there, too.

Incidentally, I thought you were saying that the returns were already diminishing significantly by 4 threads. Are you sure you want to max out at 8?

@effigies effigies reopened this Sep 12, 2025
@effigies

Copy link
Copy Markdown
Member

Sorry, I thought you'd said to close the PR, but now I can't see the comment...

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.

mris_expand is slow when using high thread count

2 participants