Skip to content

CI updates/fixes, test updates#1495

Merged
jdhughes-dev merged 1 commit into
modflowpy:developfrom
wpbonelli:tests
Aug 22, 2022
Merged

CI updates/fixes, test updates#1495
jdhughes-dev merged 1 commit into
modflowpy:developfrom
wpbonelli:tests

Conversation

@wpbonelli
Copy link
Copy Markdown
Member

@wpbonelli wpbonelli commented Aug 12, 2022

This PR:

Codecov configuration

Uploads coverage reports on pull_request as well as push events to fix inaccurate codecov bot comments on PRs.

Also:

  • don't measure & upload coverage for smoke tests, regression & example tests, and benchmarks
    • smoke tests and benchmarks are subsets of the standard test suite
    • example scripts/notebooks are not included in coverage measurements (only the flopy module is)
    • excluding regression tests does not seem to change coverage appreciably (as seems appropriate — ideally the standard test suite should exercise everything?) and I can plan to add a few cases to make up any omissions
  • decrease coverage precision from 3 to 1 decimal place to avoid small/spurious deltas
  • update to codecov-action@v3

PathlineFile performance

Fix #1479 by sorting once in the ctor instead of each call to parent's _ModpathSeries method get_data() (occurs in a loop in _MoadpathSeries.get_destination_data()).

Previous performance:

Name (time in ms) Min Max Mean StdDev Median IQR Outliers OPS Rounds Iterations
test_get_destination_pathline_data[well-backward] 26,589.8808 (>1000.0) 26,652.4284 (>1000.0) 26,623.4600 (>1000.0) 25.6051 (831.50) 26,616.2695 (>1000.0) 40.1569 (>1000.0) 2;0 0.0376 (0.00) 5 1
test_get_destination_pathline_data[well-forward] 40,028.3296 (>1000.0) 40,195.3408 (>1000.0) 40,104.5977 (>1000.0) 70.1838 (>1000.0) 40,077.0743 (>1000.0) 115.3085 (>1000.0) 2;0 0.0249 (0.00) 5 1
test_get_destination_pathline_data[river-forward] 61,112.0500 (>1000.0) 61,440.9841 (>1000.0) 61,247.5391 (>1000.0) 132.9419 (>1000.0) 61,200.5654 (>1000.0) 201.5238 (>1000.0) 2;0 0.0163 (0.00) 5 1
test_get_destination_pathline_data[river-backward] 568,730.0336 (>1000.0) 575,857.0278 (>1000.0) 571,281.7784 (>1000.0) 2,803.3254 (>1000.0) 571,121.0215 (>1000.0) 3,365.4625 (>1000.0) 1;0 0.0018 (0.00) 5 1

New benchmarks:

Name (time in ms) Min Max Mean StdDev Median IQR Outliers OPS Rounds Iterations
test_get_destination_pathline_data[well-backward] 128.9273 (120.76) 207.2077 (111.50) 153.9030 (136.77) 28.1448 (463.77) 145.9356 (130.04) 21.2064 (545.76) 1;1 6.4976 (0.01) 6
test_get_destination_pathline_data[well-forward] 171.6738 (160.80) 252.1039 (135.66) 204.7025 (181.91) 39.9782 (658.77) 181.4749 (161.71) 72.8909 (>1000.0) 1;0 4.8851 (0.01) 5
test_get_destination_pathline_data[river-forward] 238.6348 (223.52) 356.1929 (191.67) 293.0763 (260.45) 51.4346 (847.55) 269.4483 (240.10) 87.1385 (>1000.0) 2;0 3.4121 (0.00) 5
test_get_destination_pathline_data[river-backward] 2,282.6307 (>1000.0) 2,325.8112 (>1000.0) 2,303.5692 (>1000.0) 17.5329 (288.91) 2,298.4413 (>1000.0) 27.9918 (720.38) 2;0 0.4341 (0.00) 5

The operation completes in a fraction of the previous time. I don't think moving the sort() will cause problems as _ModpathSeries and children don't expose methods to update internal data after it's set in the constructor.

CI updates

Moves benchmarking, example scripts & notebook tests, and regression tests into their own workflows to allow for independent testing and scheduling. (However all three still run on the same daily schedule as before).

Benchmarks

Previously benchmark comparisons (with pytest --benchmark-compare) were failing due to an issue in the CI caching configuration. Now benchmarks are no longer saved in the cache, only as artifacts, and artifacts are postprocessed after benchmark jobs complete. The script scripts/process_benchmarks.py accepts 2 arguments, the input and output directory, and:

  • finds JSON files created by pytest with --benchmark-autosave or --benchmark-json=<file>, searching recursively in the input directory
  • collects results in a dataframe and saves it to CSV
  • creates and saves plots for each benchmarked test case

For instance, to look in the default benchmark save location autotest/.benchmarks and save results in the same place:

python scripts/process_benchmarks.py autotest/.benchmarks autotest/.benchmarks

Individual artifacts are still saved for each combination of system and python version. An aggregate artifact is created by process_benchmarks.py in a job that runs after all benchmark jobs finish (example here).

Macos/matplotlib errors

Tentatively fixes #1491 by using the non-interactive agg matplotlib backend on macos CI. Previously the macos backend was selected by default. This involves a FigureManager, which wraps a Cocoa NSWindow. Maybe something in Cocoa doesn't like being invoked from multiple processes.

Modflow exe caching

Cache modflow executables in commit-triggered and daily workflows with new composite action .github/actions/cache_exes. This installs & caches modflow executables, checking the exe repo's latest release for a code.json asset and hashing its contents if found to determine whether to invalidate the cache. Required inputs are:

  • path: the path to restore the cache to
  • github_token: the GitHub access token to authenticate API requests
Linux/MacOS

To cache Modflow executables in .local/bin in the runner's home directory, add a step like the following:

      - name: Install Modflow executables
        uses: ./.github/actions/cache_exes
        with:
          path: ~/.local/bin
          github_token: ${{ secrets.GITHUB_TOKEN }}
Windows

An equivalent step for Windows:

      - name: Install Modflow executables
        uses: ./.github/actions/cache_exes
        with:
          path: C:\Users\runneradmin\.local\bin
          github_token: ${{ secrets.GITHUB_TOKEN }}

The home directory path is explicitly provided here — although tilde expansion works in recent versions of Powershell, it does not seem to work on Windows runners.

Tests

Scripts

Rename release folder to scripts, move pull_request_prepare.py from autotest to scripts, and add new scripts for postprocessing benchmarks.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 12, 2022

Codecov Report

Merging #1495 (02bec83) into develop (c9e6f61) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 02bec83 differs from pull request most recent head 2b08442. Consider uploading reports for the commit 2b08442 to get more accurate results

@@           Coverage Diff            @@
##           develop    #1495   +/-   ##
========================================
  Coverage    72.47%   72.48%           
========================================
  Files          250      250           
  Lines        54039    54039           
========================================
+ Hits         39166    39170    +4     
+ Misses       14873    14869    -4     
Impacted Files Coverage Δ
flopy/utils/modpathfile.py 85.96% <100.00%> (ø)
flopy/plot/crosssection.py 62.42% <0.00%> (-1.56%) ⬇️
flopy/mf6/modflow/mfsimulation.py 60.46% <0.00%> (+1.70%) ⬆️

@wpbonelli wpbonelli marked this pull request as ready for review August 12, 2022 13:39
@wpbonelli wpbonelli changed the title Workaround intermittent macos CI matplotlib failures Update tests Aug 14, 2022
@wpbonelli wpbonelli marked this pull request as draft August 14, 2022 18:44
@wpbonelli wpbonelli changed the title Update tests Update tests and CI Aug 14, 2022
@wpbonelli wpbonelli changed the title Update tests and CI Macos/matplotlib CI fix, modflow executable CI caching, test updates, fix codecov Aug 14, 2022
@wpbonelli wpbonelli changed the title Macos/matplotlib CI fix, modflow executable CI caching, test updates, fix codecov CI updates/fixes, test updates, fix codecov Aug 14, 2022
@wpbonelli wpbonelli changed the title CI updates/fixes, test updates, fix codecov CI updates/fixes, test updates Aug 15, 2022
@wpbonelli wpbonelli force-pushed the tests branch 6 times, most recently from 4a0a89f to c1eede5 Compare August 16, 2022 15:14
@jdhughes-dev
Copy link
Copy Markdown
Contributor

What do the benchmark plot look like?

@wpbonelli
Copy link
Copy Markdown
Member Author

wpbonelli commented Aug 16, 2022

@jdhughes-usgs it makes one like this for each benchmarked test

test_modflow py__test_model_write_time

full example here

@wpbonelli wpbonelli force-pushed the tests branch 4 times, most recently from bef2328 to 2c54012 Compare August 16, 2022 18:18
@wpbonelli wpbonelli marked this pull request as ready for review August 16, 2022 18:54
@wpbonelli wpbonelli mentioned this pull request Aug 17, 2022
@christianlangevin
Copy link
Copy Markdown

This is looking really nice, Wes. Is this ready to merge in?

@wpbonelli
Copy link
Copy Markdown
Member Author

@langevin-usgs yes, I think it's ready.

Comment thread .github/workflows/benchmark.yml Outdated
uses: actions/cache@v3
with:
path: $HOME/.local/bin
key: modflow-exes-${{ matrix.os }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How will the cache be updated when there is a new release on the modflow-executables repo?

Here and elsewhere.

code.json in the zip files has the version numbers. We could add the code.json to the release and maybe that could be hashed so that the latest will always be used.

Copy link
Copy Markdown
Member Author

@wpbonelli wpbonelli Aug 17, 2022

Choose a reason for hiding this comment

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

Hashing code.json should work if it lives in the executables repo. Flopy CI would just need to check it out. The code.json file is currently only in the release archives though.

Another option is invalidating the cache via API per MODFLOW-ORG/executables#7.

The former would require changes to this PR, latter some changes to the executables repo PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking of adding it as a release asset but could add it as a file to the repo. If it is a release asset, it could be pulled using the GitHub API before the MODFLOW executable step in the workflows. Adding it as a file in the repo would allow us to tag the commit and tie the release to the tag. I think we would have to make a new release after adding code.json to the repo to test this option.

Copy link
Copy Markdown
Member Author

@wpbonelli wpbonelli Aug 17, 2022

Choose a reason for hiding this comment

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

The release asset option might be simplest. I'll update this PR to look for code.json in the executables repo release assets & use its hash in the cache key if it finds one, otherwise just using modflow-exes-<system> as above. That should allow caching to work now as well as if/when we do include code.json as a release asset.

Marked as draft again, will re-mark for review when done.

Copy link
Copy Markdown
Member Author

@wpbonelli wpbonelli Aug 18, 2022

Choose a reason for hiding this comment

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

Added cache_exes composite action to .github/actions. This action packages the modflow exe install so workflows can all reuse it. The action checks the latest executables release for a code.json file, and if found, hashes it to determine if there's been a new release. (This file is not yet a release asset, and the cache won't be invalidated until it becomes one.)

The cache_exes action works on ubuntu and mac runners & there's a functionally identical Windows one
The cache_exes action is cross-platform and works on Windows, Linux and Mac runners.

A further option is to move the action to its own repository so modflow6 and other repos could use them.

@wpbonelli wpbonelli marked this pull request as draft August 17, 2022 16:39
@wpbonelli wpbonelli force-pushed the tests branch 8 times, most recently from 75b9c53 to bb8ae28 Compare August 18, 2022 19:27
Comment thread .github/workflows/commit.yml Outdated
@wpbonelli wpbonelli force-pushed the tests branch 2 times, most recently from f65b4e7 to 3fa932d Compare August 18, 2022 21:57
@wpbonelli wpbonelli marked this pull request as ready for review August 18, 2022 22:32
@wpbonelli wpbonelli requested a review from jdhughes-dev August 18, 2022 22:33
@wpbonelli wpbonelli marked this pull request as draft August 19, 2022 15:01
@wpbonelli wpbonelli force-pushed the tests branch 5 times, most recently from 1fdf021 to 8c97cc3 Compare August 19, 2022 19:05
@wpbonelli wpbonelli marked this pull request as ready for review August 19, 2022 19:13
* fix ci.yml -> commit.yml
* fix modflowpy#1491: workaround intermittent macos CI matplotlib failures
* fix modflowpy#1479: sort in child's ctor instead of _ModpathSeries.get_data()
* don't plt.show() in tests
* add comments to conftest.py
* give test_mt3d.py::test_mfnwt_CrnkNic more retries
* skip ex-gwtgwt-mt3dms-p10 mf6 example (per MODFLOW-ORG/modflow6#1008)
* rename release/ to scripts/
* move pull_request_prepare.py to scripts/
* add postprocess_benchmarks.py to scripts/
* separate CI workflows for benchmarks, examples and regression tests
* name benchmark CI artifacts benchmarks-<system>-python version>-<run ID>
* add CI job to post-process benchmarks (creates artifact benchmarks-<run ID>)
* add cross-platform CI action to cache modflow exes & invalidate on new release
* reenable PathlineFile.get_destination_pathline_data() benchmark
* don't upload coverage after smoke tests, benchmarks, regression tests and example tests
* upload coverage on PR as well as push (fix codecov bot comments)
* decrease coverage precision to 1 decimal place (avoid small deltas)
* update to codecov action v3
@jdhughes-dev jdhughes-dev merged commit 58938b9 into modflowpy:develop Aug 22, 2022
wpbonelli added a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
* fix ci.yml -> commit.yml
* fix modflowpy#1491: workaround intermittent macos CI matplotlib failures
* fix modflowpy#1479: sort in child's ctor instead of _ModpathSeries.get_data()
* don't plt.show() in tests
* add comments to conftest.py
* give test_mt3d.py::test_mfnwt_CrnkNic more retries
* skip ex-gwtgwt-mt3dms-p10 mf6 example (per MODFLOW-ORG/modflow6#1008)
* rename release/ to scripts/
* move pull_request_prepare.py to scripts/
* add postprocess_benchmarks.py to scripts/
* separate CI workflows for benchmarks, examples and regression tests
* name benchmark CI artifacts benchmarks-<system>-python version>-<run ID>
* add CI job to post-process benchmarks (creates artifact benchmarks-<run ID>)
* add cross-platform CI action to cache modflow exes & invalidate on new release
* reenable PathlineFile.get_destination_pathline_data() benchmark
* don't upload coverage after smoke tests, benchmarks, regression tests and example tests
* upload coverage on PR as well as push (fix codecov bot comments)
* decrease coverage precision to 1 decimal place (avoid small deltas)
* update to codecov action v3
wpbonelli added a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
* fix ci.yml -> commit.yml
* fix modflowpy#1491: workaround intermittent macos CI matplotlib failures
* fix modflowpy#1479: sort in child's ctor instead of _ModpathSeries.get_data()
* don't plt.show() in tests
* add comments to conftest.py
* give test_mt3d.py::test_mfnwt_CrnkNic more retries
* skip ex-gwtgwt-mt3dms-p10 mf6 example (per MODFLOW-ORG/modflow6#1008)
* rename release/ to scripts/
* move pull_request_prepare.py to scripts/
* add postprocess_benchmarks.py to scripts/
* separate CI workflows for benchmarks, examples and regression tests
* name benchmark CI artifacts benchmarks-<system>-python version>-<run ID>
* add CI job to post-process benchmarks (creates artifact benchmarks-<run ID>)
* add cross-platform CI action to cache modflow exes & invalidate on new release
* reenable PathlineFile.get_destination_pathline_data() benchmark
* don't upload coverage after smoke tests, benchmarks, regression tests and example tests
* upload coverage on PR as well as push (fix codecov bot comments)
* decrease coverage precision to 1 decimal place (avoid small deltas)
* update to codecov action v3
wpbonelli added a commit that referenced this pull request Dec 14, 2022
* fix ci.yml -> commit.yml
* fix #1491: workaround intermittent macos CI matplotlib failures
* fix #1479: sort in child's ctor instead of _ModpathSeries.get_data()
* don't plt.show() in tests
* add comments to conftest.py
* give test_mt3d.py::test_mfnwt_CrnkNic more retries
* skip ex-gwtgwt-mt3dms-p10 mf6 example (per MODFLOW-ORG/modflow6#1008)
* rename release/ to scripts/
* move pull_request_prepare.py to scripts/
* add postprocess_benchmarks.py to scripts/
* separate CI workflows for benchmarks, examples and regression tests
* name benchmark CI artifacts benchmarks-<system>-python version>-<run ID>
* add CI job to post-process benchmarks (creates artifact benchmarks-<run ID>)
* add cross-platform CI action to cache modflow exes & invalidate on new release
* reenable PathlineFile.get_destination_pathline_data() benchmark
* don't upload coverage after smoke tests, benchmarks, regression tests and example tests
* upload coverage on PR as well as push (fix codecov bot comments)
* decrease coverage precision to 1 decimal place (avoid small deltas)
* update to codecov action v3
wpbonelli added a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
* fix ci.yml -> commit.yml
* fix modflowpy#1491: workaround intermittent macos CI matplotlib failures
* fix modflowpy#1479: sort in child's ctor instead of _ModpathSeries.get_data()
* don't plt.show() in tests
* add comments to conftest.py
* give test_mt3d.py::test_mfnwt_CrnkNic more retries
* skip ex-gwtgwt-mt3dms-p10 mf6 example (per MODFLOW-ORG/modflow6#1008)
* rename release/ to scripts/
* move pull_request_prepare.py to scripts/
* add postprocess_benchmarks.py to scripts/
* separate CI workflows for benchmarks, examples and regression tests
* name benchmark CI artifacts benchmarks-<system>-python version>-<run ID>
* add CI job to post-process benchmarks (creates artifact benchmarks-<run ID>)
* add cross-platform CI action to cache modflow exes & invalidate on new release
* reenable PathlineFile.get_destination_pathline_data() benchmark
* don't upload coverage after smoke tests, benchmarks, regression tests and example tests
* upload coverage on PR as well as push (fix codecov bot comments)
* decrease coverage precision to 1 decimal place (avoid small deltas)
* update to codecov action v3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Intermittent pytest-xdist failures on Mac CI runners PathlineFile.get_destination_pathline_data() performance

3 participants