Skip to content

Fix test_mass_flow_controller failures#1035

Closed
speth wants to merge 1 commit intoCantera:mainfrom
speth:debug-mfc-reactor-test
Closed

Fix test_mass_flow_controller failures#1035
speth wants to merge 1 commit intoCantera:mainfrom
speth:debug-mfc-reactor-test

Conversation

@speth
Copy link
Copy Markdown
Member

@speth speth commented May 10, 2021

Changes proposed in this pull request

  • Disable multithreaded OpenBLAS in in tests of different Sundials versions

If applicable, fill in the issue number this pull request is fixing

Closes #1033

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@bryanwweber
Copy link
Copy Markdown
Member

@speth The OpenBLAS we're getting here uses the pthread library. From the SUNDIALS 5.7 action:

openblas-0.3.15            |pthreads_h4748800_0         9.8 MB  conda-forge

I wonder if setting OMP_NUM_THREADS actually did anything in that case?

@speth
Copy link
Copy Markdown
Member Author

speth commented May 11, 2021

I had no idea that was a thing. This game of whack-a-mole is getting a little ridiculous.

@bryanwweber
Copy link
Copy Markdown
Member

Resetting this to OpenBLAS 0.3.13 seems to have fixed the error (the SUNDIALS 2 failure is due to a dependency resolution failure).

@speth
Copy link
Copy Markdown
Member Author

speth commented May 12, 2021

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.

@bryanwweber
Copy link
Copy Markdown
Member

I can't tell from the changelogs what would have affected this test: https://github.com/xianyi/OpenBLAS/releases

@speth speth force-pushed the debug-mfc-reactor-test branch from 5cc9510 to c59f048 Compare May 12, 2021 03:31
@speth speth force-pushed the debug-mfc-reactor-test branch from c59f048 to af87fdd Compare May 12, 2021 03:33
@speth speth marked this pull request as ready for review May 12, 2021 03:34
@speth speth changed the title WIP: Debugging test_mass_flow_controller Fix test_mass_flow_controller failures May 12, 2021
@speth speth requested a review from a team May 12, 2021 04:41
Copy link
Copy Markdown
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, but I’ll leave it to @bryanwweber to review & merge.

Copy link
Copy Markdown
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to include all these env_vars in the passthrough? If yes, can they go on their own line?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the env_vars option 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_vars

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@speth
Copy link
Copy Markdown
Member Author

speth commented May 17, 2021

Apparently, based on #1039, fiddling around with multithreading isn't actually a reliable solution to this problem.

@speth speth closed this May 17, 2021
@speth speth deleted the debug-mfc-reactor-test branch May 23, 2021 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sporadic failures of CI in test_mass_flow_controller

3 participants