[ENH] Limit tedana to one core

[tsalo]

References #188.

Changes proposed in this pull request:

• Set all relevant environmental variables to limit the tedana workflow to one core.

Additional information

This PR is more of a temporary fix than a permanent solution. Ultimately, we want to be able to set the number of cores with an argument. Unfortunately, every attempt I've made to do that has failed. On the bright side, there is anecdotal evidence that limiting to one core will speed the pipeline up a fair bit, although we'll want to test that to make sure it's reproducible.

This will probably upset the linter because the environmental variables have to be set before numpy and other multithreading packages are imported, which means that imports will be split at the top of the file. I'm not sure if we'll want to ignore this or to update the linter preferences to explicitly allow for it.

[rmarkello]

This looks great, @tsalo! I agree it's a bit of a band-aid solution for now, but since most users will (ideally) be running tedana via the CLI this should hopefully avoid a lot of the issues of "can't set cores if numpy is imported first."

If you'd like to add:

[flake8]
ignore = E402

to the setup.cfg in the main directory then I think we can get rid of those linter warnings and this should be good to go.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@e7106ed). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #215   +/-   ##
=========================================
  Coverage          ?   51.54%           
=========================================
  Files             ?       32           
  Lines             ?     1969           
  Branches          ?        0           
=========================================
  Hits              ?     1015           
  Misses            ?      954           
  Partials          ?        0

Impacted Files	Coverage Δ
tedana/workflows/tedana.py	14.38% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7106ed...ee8b4b3. Read the comment docs.

[rmarkello]

Interesting! I'm not totally sure why this one minor change suddenly caused all the other linter errors... I did notice that we had previously been using tox.ini for flake8 configuration, so it's possible that we overrode some settings? What do you think of the following approach:

1. Open a new PR migrating the flake8 config stuff out of tox.ini and into setup.cfg (making tox.ini obsolete);
2. Address the "new" linter errors and, assuming that passes CI, merge into master; and
3. Rebase this onto master and hopefully have no problems!

Let me know, and apologies for making this more complicated than it should have been...

[tsalo]

I'm happy to do that, but I've been testing locally and everything I do seems to disable the putty-ignore setting. I can't figure out how to make the setup.cfg (or tox.ini) ignore all of the things we want it to.

[tsalo]

I've opened a new PR with those changes (#216) so we can discuss that there.

[emdupre]

Out of curiosity, I checked the build time on Circle for these and we're not seeing any appreciable speed-boost. I wonder if that is also the case locally...

Should we make an issue about setting the number of cores programmatically, so we remember this is a (beautiful) band-aid ?

[tsalo]

It's weird that there's no difference, although maybe it's because we only have access to two cores in CircleCI? Or the OS? Either way, I would definitely like to see if the duration changes predictably over a larger range of situations, but at least it will prevent tedana jobs from blowing up HPCs.

I think that #188 is meant to cover that, although maybe @jbteves could update that issue to reflect the direction we've chosen to go in (i.e., making control explicit) or we could close that one and open a new issue specifically for the n_cores option.

[jbteves]

My kneejerk reaction is to leave it in that issue and update my comment so that the discussion history doesn't scatter; thoughts @tsalo and @emdupre?

[emdupre]

👍 to updating #188 ! Unless any other concerns, then, I'll merge this 🚀

[tsalo]

That works for me. I would recommend changing the name to something like "Add argument to set number of cores" and I can switch the label from discussion to enhancement. Thanks all!