Speed up CI tests #1025

speth · 2021-04-28T21:24:43Z

Changes proposed in this pull request

Allow a subset of "slow" tests to be disabled, and use this on the "coverage" CI runs. Mechanisms for this are introduced for Python tests, gtest tests, and the "test_problems" regression tests.
Speed up some tests by switching from CTI inputs to YAML
Avoid creating unnecessary Transport objects during tests that use gri30 and h2o2 mechanisms.

Checklist

There is a clear use-case for this code change
The commit message has a short title & references 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

ischoegl · 2021-04-28T21:32:38Z

Great to see some of this moving along! One question I had was about replacing kineticsFromScratch.cti in kineticsFromScratch.cpp (which has come up in #995). Conversion to YAML isn't the issue here (file is already in #995), but setting up the objects has created some double-free's (?) that I am unable to track down (gdb, valgrind and all). There's a parallel implementation in kineticsFromScratch3.cpp that uses the new framework ...

codecov · 2021-04-28T21:42:57Z

Codecov Report

Merging #1025 (e81c33e) into main (f27c68e) will decrease coverage by 0.03%.
The diff coverage is 95.90%.

@@            Coverage Diff             @@
##             main    #1025      +/-   ##
==========================================
- Coverage   72.57%   72.54%   -0.04%     
==========================================
  Files         364      355       -9     
  Lines       46816    46422     -394     
==========================================
- Hits        33978    33675     -303     
+ Misses      12838    12747      -91

Impacted Files	Coverage Δ
include/cantera/base/AnyMap.h	`100.00% <ø> (ø)`
src/base/plots.cpp	`0.00% <0.00%> (-92.11%)`	⬇️
test/thermo/phaseConstructors.cpp	`100.00% <ø> (ø)`
test/equil/equil_gas.cpp	`79.34% <55.55%> (-9.24%)`	⬇️
src/base/AnyMap.cpp	`89.41% <100.00%> (-0.11%)`	⬇️
src/kinetics/Reaction.cpp	`89.70% <100.00%> (+0.02%)`	⬆️
src/thermo/MaskellSolidSolnPhase.cpp	`71.69% <100.00%> (-1.20%)`	⬇️
src/thermo/SurfPhase.cpp	`82.88% <100.00%> (+0.09%)`	⬆️
test/general/test_serialization.cpp	`100.00% <100.00%> (ø)`
test/kinetics/kineticsFromScratch.cpp	`100.00% <100.00%> (ø)`
... and 48 more

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 cfa96aa...e81c33e. Read the comment docs.

speth · 2021-04-28T21:52:03Z

Great to see some of this moving along! One question I had was about replacing kineticsFromScratch.cti in kineticsFromScratch.cpp (which has come up in #995). Conversion to YAML isn't the issue here (file is already in #995), but setting up the objects has created some double-free's (?) that I am unable to track down (gdb, valgrind and all). There's a parallel implementation in kineticsFromScratch3.cpp that uses the new framework ...

Sorry, I'm not sure I understand the issue as it relates to this PR. Are you saying that if I were to substitute kineticsFromScratch.yaml for kineticsFromScratch.cti (which I haven't done yet, but would fit within the overall scope of this PR, I suppose), I should be able to see some of these errors?

ischoegl · 2021-04-28T22:06:15Z

Are you saying that if I were to substitute kineticsFromScratch.yaml for kineticsFromScratch.cti (which I haven't done yet, but would fit within the overall scope of this PR, I suppose), I should be able to see some of these errors?

Correct. My own attempt in the conversion runs without errors, but the gtest doesn't shut down properly. I am likely overlooking something in the way I set up my objects. It's the first issue in a long time that leaves me stumped. And yes, I was hoping that it would fit into the scope of this PR 😉

ischoegl · 2021-05-01T18:02:24Z

Would it be possible to add d3ee682 as a separate PR? This part of CI apparently broke yesterday, and it would be good if it were available for anyone else to rebase to.

Use YAML input format to avoid repeated calls ctml_writer

Blessed output file changed because of addition of N2 to h2o2.yaml

The CTI and XML files in test/data are still in use for tests of the conversion process. fixup! [Test] Transition use to pdep-test.yaml

Use to speed up CI coverage testing

The default phase definition for gri30.yaml and h2o2.* includes a transport model, and setting this slows down tests that don't need transport properties. For any tests changed, also switch to using the YAML versions of these input files. The addition of N2 to h2o2.yaml, along with one reaction, means that some indices had to change. This also uncovered a bug in the Gibbs equilibrium solver, which is documented in Cantera#1023.

Species are added before phase-specific parameters like site density are set. Adding the species sets their coverages, which uses the site density. Therefore, the initial value for the site density needs to be non-zero. This only affected the specific SurfPhase constructor, not methods which use ThermoFactory, since in that case the default constructor for SurfPhase gets called, which does set a non-zero site density.

For the case where a Quantity has a converter lambda function, replacing the converted value has to be done carefully to avoid accessing already-deleted memory.

Explicit tests of parsing/conversion should remain until removal of CTI/XML formats.

More consistent with the methods of standard library containers

If the phase is being created from a YAML input file, both the excess-enthalpy and product-species fields must be provided in that input.

The test IonsFromNeutralConstructor.fromXML test is made redundant by the combination of ctml2yamlTest.test_mock_ion (which include instantiation from an XML file) and ThermoFromYaml.IonsFromNeutral (which instantiates an identical phase from a YAML input file).

These reactions were unintentionally relying on having been created from YAML when serializing to YAML, where the "type" field from the input is retained by default.

fixup! [Test] Replace XML with YAML in gtest equilibrium solver tests

ischoegl

This looks good to me.

speth force-pushed the speed-up-tests branch from 58cf4b8 to ef28359 Compare April 28, 2021 22:00

speth force-pushed the speed-up-tests branch 2 times, most recently from f5ed357 to 5ae11d8 Compare May 1, 2021 17:34

speth force-pushed the speed-up-tests branch from d3ee682 to 0cc81ad Compare May 1, 2021 22:06

ischoegl mentioned this pull request May 3, 2021

Simplify/Refactor buildutils.py #1012

Merged

4 tasks

speth mentioned this pull request May 8, 2021

Refactor reaction rate evaluators #995

Merged

8 tasks

speth added 19 commits May 8, 2021 20:08

[Test] Speed up TestTwinFlame.test_restart

b475bdc

[Test] Speed up bad transport geometry test

8b762e1

Use YAML input format to avoid repeated calls ctml_writer

[Test] Speed up some reactor network tests

938c4e9

[Test] Always use YAML input in reactor tests

f36775a

Blessed output file changed because of addition of N2 to h2o2.yaml

[Test] Replace CTI input file in duplicate reaction test

da348be

[Test] Transition use to pdep-test.yaml

c7fc3b4

The CTI and XML files in test/data are still in use for tests of the conversion process. fixup! [Test] Transition use to pdep-test.yaml

[Test] Add option to skip slow (Python) tests

17047f4

Use to speed up CI coverage testing

[Test] Allow skipping of slow test_problems tests

86bdf0c

[Kinetics] Fix initialization of FalloffReaction

052f1d3

[Input] Fix error serializing Quantity with converter functions

e9fec23

For the case where a Quantity has a converter lambda function, replacing the converted value has to be done carefully to avoid accessing already-deleted memory.

[Test] Replace kineticsfromscratch.cti and sofc-test.xml with YAML

456c598

[Test] Replace CTI with YAML for BinarySolutionTabulatedThermo test

d71c880

[Test] Replace simplephases.cti with YAML

93d5548

[Test] Replace some uses of frac.cti and ptcombust.cti

9520c71

Explicit tests of parsing/conversion should remain until removal of CTI/XML formats.

[ctml2yaml] Handle trailing commas in lists

5615177

Rename "isEmpty" to "empty"

0aa3f8d

More consistent with the methods of standard library containers

[Thermo] Check for required inputs of Maskell thermo model

8430db7

If the phase is being created from a YAML input file, both the excess-enthalpy and product-species fields must be provided in that input.

speth added 12 commits May 8, 2021 20:08

[Test] Replace XML with YAML in MaskellSolidSolnPhase tests

b2a9b8f

[Test] Replace CTI with YAML in RedlichKwong test

5974ecf

[Test] Convert gasNASA9.xml to YAML

19c4599

[Test] Migrate ThermoYamlRoundTrip tests from XML to YAML

1862835

[Kinetics] Fix serialization of Blowers-Masel rates

bd34d23

These reactions were unintentionally relying on having been created from YAML when serializing to YAML, where the "type" field from the input is retained by default.

[Base] Deprecate rarely-used file output functions

74f93b5

[Test] Replace XML with YAML in more gtest tests

4457eae

fixup! [Test] Replace XML with YAML in gtest equilibrium solver tests

[Test] Remove dead code from ThermoPhase_Test.cpp

7e873ef

[Test] Replace XML with YAML for ReactionToYaml tests

b144b45

[Python] Fix compilation with Cython 3.0a6

95a1da7

[Test] Add ability to skip slow GTest tests

e81c33e

speth force-pushed the speed-up-tests branch from 0cc81ad to e81c33e Compare May 9, 2021 00:08

speth marked this pull request as ready for review May 9, 2021 00:47

ischoegl approved these changes May 9, 2021

View reviewed changes

speth merged commit 19f1593 into Cantera:main May 9, 2021

speth deleted the speed-up-tests branch May 9, 2021 22:08

ischoegl mentioned this pull request May 10, 2021

Sporadic failures of CI in test_mass_flow_controller #1033

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up CI tests #1025

Speed up CI tests #1025

speth commented Apr 28, 2021 •

edited

Loading

ischoegl commented Apr 28, 2021 •

edited

Loading

codecov bot commented Apr 28, 2021 •

edited

Loading

speth commented Apr 28, 2021

ischoegl commented Apr 28, 2021 •

edited

Loading

ischoegl commented May 1, 2021 •

edited

Loading

ischoegl left a comment

Speed up CI tests #1025

Speed up CI tests #1025

Conversation

speth commented Apr 28, 2021 • edited Loading

ischoegl commented Apr 28, 2021 • edited Loading

codecov bot commented Apr 28, 2021 • edited Loading

Codecov Report

speth commented Apr 28, 2021

ischoegl commented Apr 28, 2021 • edited Loading

ischoegl commented May 1, 2021 • edited Loading

ischoegl left a comment

Choose a reason for hiding this comment

speth commented Apr 28, 2021 •

edited

Loading

ischoegl commented Apr 28, 2021 •

edited

Loading

codecov bot commented Apr 28, 2021 •

edited

Loading

ischoegl commented Apr 28, 2021 •

edited

Loading

ischoegl commented May 1, 2021 •

edited

Loading