From e4e9eb4d67fb60c2fd75c8d3b0cd65b194a0024b Mon Sep 17 00:00:00 2001 From: Jackson Burns Date: Mon, 17 Jul 2023 21:53:32 -0400 Subject: [PATCH 1/5] continuous integration should pass by default - only test solving environment and building RMG on Mac, no testing (we distribute on ubuntu anyway) - remove requirement for regression comparisons to pass - comment the results to the PR - beep boop :robot: --- .github/workflows/CI.yml | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 78a48b51cac..3230ab0b1c2 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -26,6 +26,7 @@ # 2023-06-07 - updated regression testing. Now fails if significant changes are detected. # 2023-06-15 - revert changes from 06-06, both now work # 2023-06-27 - add option to run from RMG-database with GitHub resuable workflows +# 2023-07-17 - made it pass by default name: Continuous Integration on: @@ -56,7 +57,6 @@ jobs: matrix: os: [ubuntu-latest, macos-latest] runs-on: ${{ matrix.os }} - continue-on-error: ${{ matrix.os == 'macos-latest' }} # skip scheduled runs from forks if: ${{ !( github.repository != 'ReactionMechanismGenerator/RMG-Py' && github.event_name == 'schedule' ) }} env: @@ -118,6 +118,10 @@ jobs: make clean make + - name: Exit for MacOS + if: ${{ matrix.os == 'macos-latest' }} + run: exit 0 + # RMS installation and linking to Julia # Allow these installs to 'fail' (as they do in RMG-Tests) with the command || True trick - name: Install and link Julia dependencies @@ -159,7 +163,7 @@ jobs: if: ${{ failure() && steps.regression-execution.conclusion == 'failure' }} uses: actions/upload-artifact@v3 with: - name: failed regression results ${{ matrix.os }} + name: failed regression results path: | test/regression @@ -178,7 +182,7 @@ jobs: if: ${{ env.REFERENCE_JOB == 'false' }} uses: actions/upload-artifact@v3 with: - name: dynamic regression results ${{ matrix.os }} + name: dynamic regression results path: | test/regression @@ -274,16 +278,33 @@ jobs: if [[ ${FAILED} ]]; then echo "One or more regression tests failed." | tee -a $GITHUB_STEP_SUMMARY echo "Please download the failed results and run the tests locally or check the above log to see why." | tee -a $GITHUB_STEP_SUMMARY - exit 1 fi - # Upload Regression Results as Failed if above step failed - - name: Upload Failed Comparison Results - if: ${{ failure() && steps.regression-comparison.conclusion == 'failure' }} + + - name: Prepare Results for PR Comment + if : ${{ github.event_name == 'pull_request' }} + run: | + echo "
" > summary.txt + echo "Regression Testing Results" >> summary.txt + echo "" >> summary.txt + tail -n +1 test/regression-diff/* >> summary.txt + echo "" + echo "
" >> summary.txt + echo "" >> summary.txt + echo "_beep boop this action was performed by a bot_ :robot:" >> summary.txt + + - name: Write Results to PR + uses: thollander/actions-comment-pull-request@v2 + if : ${{ github.event_name == 'pull_request' }} + with: + filePath: summary.txt + + - name: Upload Comparison Results uses: actions/upload-artifact@v3 with: - name: failed regression comparison results ${{ matrix.os }} + name: Regression Test Comparison Results path: | test/regression-diff + # Install and Call codecov only if the tests were successful (permitting failures in the regression comparison tests) - name: Code coverage install and run if: success() || ( failure() && steps.regression-execution.conclusion == 'success' ) From 6948a30e184e00c45bafaa24fdb0c184c4bdc549 Mon Sep 17 00:00:00 2001 From: Richard West Date: Tue, 18 Jul 2023 13:28:12 -0400 Subject: [PATCH 2/5] When diffing models only print surface mechanism warning when needed. This was a minor bug. --- rmgpy/tools/diffmodels.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rmgpy/tools/diffmodels.py b/rmgpy/tools/diffmodels.py index 93d66740984..62a2c68e728 100644 --- a/rmgpy/tools/diffmodels.py +++ b/rmgpy/tools/diffmodels.py @@ -352,8 +352,8 @@ def execute(chemkin1, species_dict1, thermo1, chemkin2, species_dict2, thermo2, model2.species, model2.reactions = load_chemkin_file( chemkin2, species_dict2, thermo_path=thermo2, surface_path=surface_path2) except KeyError: - if 'surface_path1' in kwargs or 'surface_path2': - logging.warning('please specify 2 surface input files if you are comparing a surface mechanism') + if 'surface_path1' in kwargs or 'surface_path2' in kwargs: + logging.warning('Please specify 2 surface input files if you are comparing a surface mechanism') model1.species, model1.reactions = load_chemkin_file(chemkin1, species_dict1, thermo_path=thermo1) model2.species, model2.reactions = load_chemkin_file(chemkin2, species_dict2, thermo_path=thermo2) From 013b9ae274255abeaf6bc7c3646857ccb496e77d Mon Sep 17 00:00:00 2001 From: Richard West Date: Tue, 18 Jul 2023 13:31:31 -0400 Subject: [PATCH 3/5] Formatting of regression test summaries. Made tables, reactions, species, into markdown, and added some emoji. These reports now get pasted into the github pull request comments by a bot, so trying to make them look nice once formatted by github-flavored-markdown. --- rmgpy/tools/observablesregression.py | 12 ++--- scripts/checkModels.py | 71 +++++++++++++++------------- 2 files changed, 43 insertions(+), 40 deletions(-) diff --git a/rmgpy/tools/observablesregression.py b/rmgpy/tools/observablesregression.py index 46135f7485a..7474cd22e49 100644 --- a/rmgpy/tools/observablesregression.py +++ b/rmgpy/tools/observablesregression.py @@ -253,12 +253,12 @@ def compare(self, tol, plot=False): if old_rmg_species: variable_old = next((data for data in data_list_old if data.species == old_rmg_species), None) else: - print('No RMG species found for observable species {0} in old model.'.format(smiles)) + print('⁉️ No RMG species found for observable species {0} in old model.'.format(smiles)) fail = True if new_rmg_species: variable_new = next((data for data in data_list_new if data.species == new_rmg_species), None) else: - print('No RMG species found for observable species {0} in new model.'.format(smiles)) + print('⁉️ No RMG species found for observable species {0} in new model.'.format(smiles)) fail = True if fail is False: @@ -281,7 +281,7 @@ def compare(self, tol, plot=False): fail_header_printed = True if i not in conditions_broken: conditions_broken.append(i) - print("Observable species {0} varied by more than {1:.3f} on average between old model {2} and " + print("❌ Observable species {0} varied by more than {1:.3f} on average between old model {2} and " "new model {3} in condition {4:d}.".format(smiles, tol, variable_old.label, variable_new.label, i + 1)) variables_failed.append((self.conditions[i], smiles, variable_old, variable_new)) @@ -298,7 +298,7 @@ def compare(self, tol, plot=False): fail_header_printed = True print(fail_header) - print("Observable variable {0} varied by more than {1:.3f} on average between old model and " + print("❌ Observable variable {0} varied by more than {1:.3f} on average between old model and " "new model in condition {2:d}.".format(variable_old.label, tol, i + 1)) variables_failed.append((self.conditions[i], varName, variable_old, variable_new)) @@ -315,7 +315,7 @@ def compare(self, tol, plot=False): if fail_header_printed: print('') - print('The following reaction conditions had some discrepancies:') + print('⚠️ The following reaction conditions had some discrepancies:') for index in conditions_broken: print("Condition {0:d}:".format(index + 1)) print(str(self.conditions[index])) @@ -323,7 +323,7 @@ def compare(self, tol, plot=False): return variables_failed else: print('') - print('All Observables varied by less than {0:.3f} on average between old model and ' + print('✅ All Observables varied by less than {0:.3f} on average between old model and ' 'new model in all conditions!'.format(tol)) print('') diff --git a/scripts/checkModels.py b/scripts/checkModels.py index 23c0a2489ea..9d37cb3c6e7 100644 --- a/scripts/checkModels.py +++ b/scripts/checkModels.py @@ -110,13 +110,18 @@ def checkModel(commonSpecies, uniqueSpeciesTest, uniqueSpeciesOrig, commonReacti test_model_species = len(commonSpecies) + len(uniqueSpeciesTest) orig_model_species = len(commonSpecies) + len(uniqueSpeciesOrig) - logger.error('Test model has {} species.'.format(test_model_species)) - logger.error('Original model has {} species.'.format(orig_model_species)) + logger.error(f"Original model has {orig_model_species} species.") + logger.error(f"Test model has {test_model_species} species." + + '✅' if test_model_species == orig_model_species else '❌') + test_model_rxns = len(commonReactions) + len(uniqueReactionsTest) orig_model_rxns = len(commonReactions) + len(uniqueReactionsOrig) - logger.error('Test model has {} reactions.'.format(test_model_rxns)) - logger.error('Original model has {} reactions.'.format(orig_model_rxns)) + + logger.error(f"Original model has {orig_model_rxns} reactions.") + logger.error(f"Test model has {test_model_rxns} reactions." + + '✅' if test_model_rxns == orig_model_rxns else '❌') + return (test_model_species != orig_model_species) or (test_model_rxns != orig_model_rxns) @@ -127,38 +132,40 @@ def checkSpecies(commonSpecies, uniqueSpeciesTest, uniqueSpeciesOrig): # check for unique species in one of the models: if uniqueSpeciesOrig: error = True - logger.error('The original model has {} species that the tested model ' - 'does not have.'.format(len(uniqueSpeciesOrig))) + logger.error(f'The original model has {len(uniqueSpeciesOrig)} species ' + 'that the tested model does not have. ❌') [printSpecies(spc) for spc in uniqueSpeciesOrig] if uniqueSpeciesTest: error = True - logger.error('The tested model has {} species that the original model ' - 'does not have.'.format(len(uniqueSpeciesTest))) + logger.error(f'The tested model has {len(uniqueSpeciesTest)} species ' + 'that the original model does not have. ❌') [printSpecies(spc) for spc in uniqueSpeciesTest] # check for different thermo among common species:: if commonSpecies: for spec1, spec2 in commonSpecies: - logger.info(' {0!s}'.format(spec1)) + logger.info(' %s', spec1) if spec1.thermo and spec2.thermo: if not spec1.thermo.is_similar_to(spec2.thermo): error = True logger.error('') - logger.error('Non-identical thermo!') - logger.error('original:\t{}'.format(spec1.label)) - logger.error('tested:\t{}'.format(spec2.label)) - logger.error("{0:10}|{1:10}|{2:10}|{3:10}|{4:10}|{5:10}|{6:10}|{7:10}|{8:10}" + logger.error('Non-identical thermo! ❌') + logger.error(f'original: `{spec1.label}`') + logger.error(f'tested: `{spec2.label}`') + logger.error("|{0:10}|{1:10}|{2:10}|{3:10}|{4:10}|{5:10}|{6:10}|{7:10}|{8:10}|" .format('Hf(300K)', 'S(300K)', 'Cp(300K)', 'Cp(400K)', 'Cp(500K)', 'Cp(600K)', 'Cp(800K)', 'Cp(1000K)', 'Cp(1500K)') ) + logger.error("|----------|----------|----------|----------|----------|----------|----------|----------|----------|") [printThermo(spc) for spc in [spec1, spec2]] if spec1.thermo.comment != spec2.thermo.comment: [printSpeciesComments(spc) for spc in [spec1, spec2]] else: - logger.error('Identical thermo comments') + logger.error('Identical thermo comments:') + printSpeciesComments(spec1) return error @@ -170,37 +177,33 @@ def checkReactions(commonReactions, uniqueReactionsTest, uniqueReactionsOrig): if uniqueReactionsOrig: error = True - logger.error('The original model has {} reactions that the tested model ' - 'does not have.'.format(len(uniqueReactionsOrig))) - + logger.error(f'The original model has {len(uniqueReactionsOrig)} reactions ' + 'that the tested model does not have. ❌') [printReaction(rxn) for rxn in uniqueReactionsOrig] if uniqueReactionsTest: error = True - - logger.error('The tested model has {} reactions that the original model ' - 'does not have.'.format(len(uniqueReactionsTest))) - + logger.error(f'The tested model has {len(uniqueReactionsTest)} reactions ' + 'that the original model does not have. ❌') [printReaction(rxn) for rxn in uniqueReactionsTest] if commonReactions: for rxn1, rxn2 in commonReactions: - logger.info(' {0!s}'.format(rxn1)) + logger.info(' %s', rxn1) if rxn1.kinetics and rxn2.kinetics: if not rxn1.kinetics.is_similar_to(rxn2.kinetics): error = True logger.error('') - logger.error('Non-identical kinetics!') + logger.error('Non-identical kinetics! ❌') logger.error('original:') printReaction(rxn1) logger.error('tested:') printReaction(rxn2) - logger.error("{0:7}|{1:7}|{2:7}|{3:7}|{4:7}|{5:7}|{6:7}|{7:7}|{8:7}" + logger.error("|{0:7}|{1:7}|{2:7}|{3:7}|{4:7}|{5:7}|{6:7}|{7:7}|{8:7}|" .format('k(1bar)', '300K', '400K', '500K', '600K', '800K', '1000K', '1500K', '2000K') ) - - logger.error('') + logger.error("|-------|-------|-------|-------|-------|-------|-------|-------|-------|") [printRates(rxn) for rxn in [rxn1, rxn2]] logger.error('') @@ -209,7 +212,8 @@ def checkReactions(commonReactions, uniqueReactionsTest, uniqueReactionsOrig): if rxn1.kinetics.comment != rxn2.kinetics.comment: [printReactionComments(rxn) for rxn in [rxn1, rxn2]] else: - logger.error('Identical kinetics comments') + logger.error('Identical kinetics comments:') + printReactionComments(rxn1) return error @@ -218,7 +222,6 @@ def printSpecies(spc): """ """ - logger.error( 'spc: {}'.format(spc) ) @@ -226,9 +229,9 @@ def printSpecies(spc): def printRates(rxn): """ - + Print the rate coefficients of a reaction at various temperatures, in a markdown table. """ - logger.error("{0:7}|{1:7.2f}|{2:7.2f}|{3:7.2f}|{4:7.2f}|{5:7.2f}|{6:7.2f}|{7:7.2f}|{8:7.2f}" + logger.error("|{0:7}|{1:7.2f}|{2:7.2f}|{3:7.2f}|{4:7.2f}|{5:7.2f}|{6:7.2f}|{7:7.2f}|{8:7.2f}|" .format( 'k(T): ', math.log10(rxn.kinetics.get_rate_coefficient(300, 1e5)), @@ -244,9 +247,9 @@ def printRates(rxn): def printThermo(spec): """ - + Print the thermo of a species at various temperatures, in a markdown table. """ - logger.error("{0:10.2f}|{1:10.2f}|{2:10.2f}|{3:10.2f}|{4:10.2f}|{5:10.2f}|{6:10.2f}|{7:10.2f}|{8:10.2f}" + logger.error("|{0:10.2f}|{1:10.2f}|{2:10.2f}|{3:10.2f}|{4:10.2f}|{5:10.2f}|{6:10.2f}|{7:10.2f}|{8:10.2f}|" .format( spec.thermo.get_enthalpy(300) / 4184., spec.thermo.get_entropy(300) / 4.184, @@ -261,7 +264,7 @@ def printThermo(spec): def printReaction(rxn): - logger.error('rxn: {}\t\torigin: {}'.format(rxn, rxn.get_source())) + logger.error('rxn: `{}`\t\torigin: {}'.format(rxn, rxn.get_source())) def printReactionComments(rxn): @@ -273,7 +276,7 @@ def printSpeciesComments(spc): def printKinetics(rxn): - logger.error('Kinetics: {}'.format(rxn.kinetics)) + logger.error('kinetics: `{}`'.format(rxn.kinetics)) def initialize_log(verbose, log_file_name='checkModels.log'): From 9ccfbf841edce9a3da732561c348f35735654e3f Mon Sep 17 00:00:00 2001 From: Jackson Burns Date: Tue, 18 Jul 2023 16:21:41 -0400 Subject: [PATCH 4/5] mac runner gets own job to allow early exit after build check --- .github/workflows/CI.yml | 83 ++++++++++++++++++++++++++++++++-------- 1 file changed, 68 insertions(+), 15 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 3230ab0b1c2..2fa92ec72c9 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -52,11 +52,68 @@ concurrency: cancel-in-progress: true jobs: - build-and-test-unix: - strategy: - matrix: - os: [ubuntu-latest, macos-latest] - runs-on: ${{ matrix.os }} + build-osx: + runs-on: macos-latest + # skip scheduled runs from forks + if: ${{ !( github.repository != 'ReactionMechanismGenerator/RMG-Py' && github.event_name == 'schedule' ) }} + env: + # if running on RMG-Py but requiring changes on an un-merged branch of RMG-database, replace + # main with the name of the branch + RMG_DATABASE_BRANCH: main + defaults: + run: + shell: bash -l {0} + steps: + - name: Checkout RMG-Py + uses: actions/checkout@v3 + with: + repository: ReactionMechanismGenerator/RMG-Py + + # configures the mamba environment manager and builds the environment + - name: Setup Mambaforge Python 3.7 + uses: conda-incubator/setup-miniconda@v2 + with: + environment-file: environment.yml + miniforge-variant: Mambaforge + miniforge-version: latest + python-version: 3.7 + activate-environment: rmg_env + use-mamba: true + + # list the environment for debugging purposes + - name: mamba info + run: | + mamba info + mamba list + + # Clone RMG-database + - name: Clone RMG-database - Reusable Workflow + if: github.repository == 'ReactionMechanismGenerator/RMG-database' + run: | + cd .. + git clone -b ${{ inputs.rmg-db-branch }} https://github.com/ReactionMechanismGenerator/RMG-database.git + + - name: Clone RMG-database - RMG-Py + if: github.repository != 'ReactionMechanismGenerator/RMG-database' + run: | + cd .. + git clone -b $RMG_DATABASE_BRANCH https://github.com/ReactionMechanismGenerator/RMG-database.git + + # modify env variables as directed in the RMG installation instructions + - name: Set Environment Variables + run: | + RUNNER_CWD=$(pwd) + echo "PYTHONPATH=$RUNNER_CWD/RMG-Py:$PYTHONPATH" >> $GITHUB_ENV + echo "$RUNNER_CWD/RMG-Py" >> $GITHUB_PATH + + # RMG build step + - name: make RMG + run: | + make clean + make + + build-and-test-linux: + runs-on: ubuntu-latest # skip scheduled runs from forks if: ${{ !( github.repository != 'ReactionMechanismGenerator/RMG-Py' && github.event_name == 'schedule' ) }} env: @@ -65,7 +122,7 @@ jobs: RMG_DATABASE_BRANCH: main # This is true only if this is a reference case for the regression testing: - REFERENCE_JOB: ${{ github.ref == 'refs/heads/main' && matrix.os =='ubuntu-latest' && github.repository == 'ReactionMechanismGenerator/RMG-Py' }} + REFERENCE_JOB: ${{ github.ref == 'refs/heads/main' && github.repository == 'ReactionMechanismGenerator/RMG-Py' }} defaults: run: shell: bash -l {0} @@ -118,10 +175,6 @@ jobs: make clean make - - name: Exit for MacOS - if: ${{ matrix.os == 'macos-latest' }} - run: exit 0 - # RMS installation and linking to Julia # Allow these installs to 'fail' (as they do in RMG-Tests) with the command || True trick - name: Install and link Julia dependencies @@ -163,7 +216,7 @@ jobs: if: ${{ failure() && steps.regression-execution.conclusion == 'failure' }} uses: actions/upload-artifact@v3 with: - name: failed regression results + name: failed_regression_results path: | test/regression @@ -182,7 +235,7 @@ jobs: if: ${{ env.REFERENCE_JOB == 'false' }} uses: actions/upload-artifact@v3 with: - name: dynamic regression results + name: dynamic_regression_results path: | test/regression @@ -301,7 +354,7 @@ jobs: - name: Upload Comparison Results uses: actions/upload-artifact@v3 with: - name: Regression Test Comparison Results + name: regression_test_comparison_results path: | test/regression-diff @@ -319,7 +372,7 @@ jobs: # who knows ¯\_(ツ)_/¯ # # taken from https://github.com/docker/build-push-action - needs: build-and-test-unix + needs: build-and-test-linux runs-on: ubuntu-latest if: github.ref == 'refs/heads/main' && github.repository == 'ReactionMechanismGenerator/RMG-Py' steps: @@ -339,4 +392,4 @@ jobs: uses: docker/build-push-action@v4 with: push: true - tags: reactionmechanismgenerator/rmg:latest + tags: reactionmechanismgenerator/rmg:latest \ No newline at end of file From cd575f920d676a500ee7636e55f1c2c5112426e7 Mon Sep 17 00:00:00 2001 From: Richard West Date: Tue, 18 Jul 2023 22:37:09 -0400 Subject: [PATCH 5/5] Tweak markdown formatting of regression comparisons. --- scripts/checkModels.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/scripts/checkModels.py b/scripts/checkModels.py index 9d37cb3c6e7..65d1a70f8e9 100644 --- a/scripts/checkModels.py +++ b/scripts/checkModels.py @@ -158,9 +158,8 @@ def checkSpecies(commonSpecies, uniqueSpeciesTest, uniqueSpeciesOrig): 'Cp(800K)', 'Cp(1000K)', 'Cp(1500K)') ) logger.error("|----------|----------|----------|----------|----------|----------|----------|----------|----------|") - [printThermo(spc) for spc in [spec1, spec2]] - + logger.error('') if spec1.thermo.comment != spec2.thermo.comment: [printSpeciesComments(spc) for spc in [spec1, spec2]] else: @@ -272,7 +271,7 @@ def printReactionComments(rxn): def printSpeciesComments(spc): - logger.error('thermo: {}'.format(spc.thermo.comment)) + logger.error('thermo: {}'.format(spc.thermo.comment.replace('\n',' '))) def printKinetics(rxn):