Conversation
|
@speth The OpenBLAS we're getting here uses the I wonder if setting |
|
I had no idea that was a thing. This game of whack-a-mole is getting a little ridiculous. |
|
Resetting this to OpenBLAS 0.3.13 seems to have fixed the error (the SUNDIALS 2 failure is due to a dependency resolution failure). |
|
OK, I forced the tests to run again, and I believe you that it's something to do with the change in the OpenBLAS version. |
|
I can't tell from the changelogs what would have affected this test: https://github.com/xianyi/OpenBLAS/releases |
5cc9510 to
c59f048
Compare
c59f048 to
af87fdd
Compare
ischoegl
left a comment
There was a problem hiding this comment.
Excellent, but I’ll leave it to @bryanwweber to review & merge.
bryanwweber
left a comment
There was a problem hiding this comment.
This is simple enough, just one question.
| scons build extra_inc_dirs=$CONDA_PREFIX/include:$CONDA_PREFIX/include/eigen3 \ | ||
| extra_lib_dirs=$CONDA_PREFIX/lib system_fmt=y system_eigen=y system_yamlcpp=y \ | ||
| system_sundials=y blas_lapack_libs='lapack,blas' -j2 VERBOSE=True debug=n | ||
| system_sundials=y blas_lapack_libs='lapack,blas' -j2 VERBOSE=True debug=n env_vars=PATH,LD_LIBRARY_PATH,PYTHONPATH,OPENBLAS_NUM_THREADS |
There was a problem hiding this comment.
Is it necessary to include all these env_vars in the passthrough? If yes, can they go on their own line?
There was a problem hiding this comment.
That's the default list (which would be superseded by any setting for env_vars), plus the one new one, so this is the minimal perturbation to the build environment. I haven't really given consideration to whether any of the others could be left off for this CI job.
There was a problem hiding this comment.
Would it make more sense to add *_NUM_THREADS to the environment variable passthrough in SConstruct? I foresee this as being something that would be useful in other testing scenarios, even locally.
There was a problem hiding this comment.
I believe having more thorough tests for multi-threaded operation is an interesting thought. Thus far, it was my impression that multi-threading worked for reactor networks automatically and reliably, which this episode somehow puts in question. That said, I believe the fix for CI is ok as it stands, but it may make sense to address multi-threading in a more strategic fashion in a follow up feature PR.
There was a problem hiding this comment.
the env_vars option doesn't support wildcards, so unless you mean listing all of half dozen or more options, not really. There is the option of passing through all environment variables (env_vars=all), but that kind of goes against the SCons philosophy of not passing this information through to keep the build repeatable.
I don't think it would be productive to do any specific testing of multithreaded code that is two layers abstracted from Cantera (with SUNDIALS sitting in the middle, at least in this case), and where we have basically no control over what library the user may ultimately have linked to. I have trouble imagining a scenario in which a problem that arises in such a scenario would be something that could be addressed within Cantera.
There was a problem hiding this comment.
the
env_varsoption doesn't support wildcards
We already do some processing of the env_vars option, though... We could do something like:
import re
import os
r = re.compile(".*_NUM_THREADS")
num_threads = list(filter(r.match, os.environ.keys()))
for n in num_threads:
# add the options to env_varsThere was a problem hiding this comment.
You want to include these variables all the time, not just when they've been specified as part of env_vars? If you want to open a different PR, be my guest.
|
Apparently, based on #1039, fiddling around with multithreading isn't actually a reliable solution to this problem. |
Changes proposed in this pull request
If applicable, fill in the issue number this pull request is fixing
Closes #1033
Checklist
scons build&scons test) and unit tests address code coverage