Turn off implicit multithreading via env vars for all uses of sconsUtils #118
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
- Coverage 87.50% 85.71% -1.79%
==========================================
Files 5 5
Lines 32 42 +10
Branches 2 3 +1
==========================================
+ Hits 28 36 +8
- Misses 2 3 +1
- Partials 2 3 +1 ☔ View full report in Codecov by Sentry. |
python/lsst/sconsUtils/state.py
Outdated
| OPENBLAS_NUM_THREADS | ||
| NUMEXPR_MAX_THREADS | ||
| NUMEXPR_NUM_THREADS | ||
| """.split() |
There was a problem hiding this comment.
Why this way instead of making a list directly?
There was a problem hiding this comment.
I'm following the code above ... when in Rome.
There was a problem hiding this comment.
Why the heck were they done that way?
python/lsst/sconsUtils/state.py
Outdated
| @@ -215,6 +215,14 @@ def _initEnvironment(): | |||
| CC_LOGGER_BIN | |||
| """.split() | |||
|
|
|||
| implicitMultithreadingVars = """ | |||
There was a problem hiding this comment.
Please put a comment here about why you set these and how you found them.
There was a problem hiding this comment.
I have updated the list of variables to be more complete from lsst.utils even covering less common env vars.
| def testNoImplicitMultithreading(self): | ||
| """Test that the environment has turned off implicit | ||
| multithreading. | ||
| """ | ||
| envVar = "OMP_NUM_THREADS" |
There was a problem hiding this comment.
Is there anything you could do in a test to try to spawn extra threads and confirm that it doesn't?
There was a problem hiding this comment.
So I don't know exactly that, but I did add a test to check the number of threads that numexpr thinks it has (following lsst.utils)
erykoff
force-pushed
the
tickets/DM-42799
branch
from
06d19e0 to
9fefedb
Compare
| try: | ||
| import numexpr | ||
| except ImportError: | ||
| numexpr = None |
There was a problem hiding this comment.
| try: | |
| import numexpr | |
| except ImportError: | |
| numexpr = None | |
| try: | |
| import numexpr | |
| self.assertEqual(numexpr.utils.get_num_threads(), 1) | |
| except ImportError: | |
| # It's not a problem if numexpr isn't available. | |
| pass |
There was a problem hiding this comment.
I don't like putting extra code in the try block.
There was a problem hiding this comment.
Eh, the ImportError can only be raised by one thing in it... I don't care that much, but I think it's simpler this way.
Although: when would numexpr not be available in the environment?
There was a problem hiding this comment.
I don't know if sconsUtils is something that we ever may release separately and it doesn't really depend on numexpr, so I thought this was safer. And I didn't want to do the import up top because of all the reasons that this is necessary in the first place.
This is an env var banhammer to turn off implicit multithreading before any processes are spawned which may create threads on startup (I'm looking at you pytest plugins that import numpy on registration...)