Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make CI + tests more efficient #749

Merged
merged 7 commits into from
Dec 13, 2024
Merged

Make CI + tests more efficient #749

merged 7 commits into from
Dec 13, 2024

Conversation

penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Dec 12, 2024

This PR does a bunch of individually minor things, detailed below, but the broad effect is:

  • The time taken to run DynamicPPL CI should be cut down to ca. 30 minutes. It was close to 2 hours previously.

  • We should get back the green tick so that we can stop force-merging everything.

  • Fixes x86 2threads ubuntu CI OOM failure #725

CI

  • Separates the tests into two different groups. This was meant as a solution to x86 2threads ubuntu CI OOM failure #725, because the OOM error doesn't happen if you only run the doctests and not the main test suite before it.

  • Disables fail-fast on the CI matrix, as we don't want half the tests to fail if there's an error in the other half.

  • Enables concurrency on the CI job so that pushing a new commit to the same PR cancels CI running on previous commits

Tests themselves

  • Suppresses unnecessary output in the test suite (sampling progress, also the result of model_warntype)

  • Add verbose = true to the top-level @testset so that a breakdown of timings / number of tests is printed at the end

  • Removes one layer of an unneeded double loop over the demo models in test/sampler.jl, which cuts approximately 45 mins from CI.

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.94%. Comparing base (5c8fc59) to head (0c266a6).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #749      +/-   ##
==========================================
- Coverage   86.02%   85.94%   -0.09%     
==========================================
  Files          36       36              
  Lines        4272     4276       +4     
==========================================
  Hits         3675     3675              
- Misses        597      601       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coveralls
Copy link

coveralls commented Dec 12, 2024

Pull Request Test Coverage Report for Build 12302264036

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1818 unchanged lines in 27 files lost coverage.
  • Overall coverage decreased (-44.5%) to 41.991%

Files with Coverage Reduction New Missed Lines %
ext/DynamicPPLZygoteRulesExt.jl 4 0.0%
src/selector.jl 5 0.0%
src/model.jl 6 87.04%
src/test_utils/model_interface.jl 7 0.0%
ext/DynamicPPLForwardDiffExt.jl 13 0.0%
src/logdensityfunction.jl 15 38.24%
src/distribution_wrappers.jl 16 8.57%
src/values_as_in_model.jl 16 35.19%
src/extract_priors.jl 16 48.39%
src/model_utils.jl 17 7.14%
Totals Coverage Status
Change from base Build 12295360310: -44.5%
Covered Lines: 1759
Relevant Lines: 4189

💛 - Coveralls

@coveralls
Copy link

coveralls commented Dec 12, 2024

Pull Request Test Coverage Report for Build 12315584178

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1172 unchanged lines in 23 files lost coverage.
  • Overall coverage remained the same at 86.025%

Files with Coverage Reduction New Missed Lines %
src/selector.jl 3 42.86%
src/test_utils/varinfo.jl 5 78.26%
src/test_utils/model_interface.jl 5 22.22%
src/distribution_wrappers.jl 6 45.71%
src/model.jl 6 87.96%
src/logdensityfunction.jl 6 61.76%
src/model_utils.jl 7 7.14%
ext/DynamicPPLMCMCChainsExt.jl 8 45.45%
src/test_utils/contexts.jl 12 58.14%
src/pointwise_logdensities.jl 14 72.48%
Totals Coverage Status
Change from base Build 12308649280: 0.0%
Covered Lines: 3675
Relevant Lines: 4272

💛 - Coveralls

@penelopeysm penelopeysm marked this pull request as draft December 12, 2024 23:52
@penelopeysm penelopeysm force-pushed the py/separate-doctests branch 3 times, most recently from 9b05e78 to 3042394 Compare December 13, 2024 01:29
@penelopeysm penelopeysm changed the title Small reworking of test suite Make CI + tests more efficient Dec 13, 2024
@penelopeysm
Copy link
Member Author

penelopeysm commented Dec 13, 2024

I think this PR should be reviewable!

Coverage is the other thing that frankly annoys me. I don't see how my changes could have led to any decrease in coverage. That said, at least it's not part of the 'required checks', so it wouldn't mess with the merge queue.

Note: The 'required checks' below refer to the CI jobs that existed prior to this PR, i.e. the tests that weren't split up. The 'expected check' is just GitHub waiting for a job with that name to be run. If this PR is merged, those required checks will have to be edited in the repo settings.

@penelopeysm penelopeysm marked this pull request as ready for review December 13, 2024 02:41
test/debug_utils.jl Outdated Show resolved Hide resolved
test/runtests.jl Show resolved Hide resolved
test/sampler.jl Show resolved Hide resolved
@penelopeysm penelopeysm requested a review from mhauru December 13, 2024 15:21
Copy link
Member

@mhauru mhauru left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Penny!

@penelopeysm penelopeysm merged commit 3077a5e into master Dec 13, 2024
19 of 20 checks passed
@penelopeysm penelopeysm deleted the py/separate-doctests branch December 13, 2024 17:31
@penelopeysm
Copy link
Member Author

Branch protection rules updated too so that should show up right on the next PR :)

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.

x86 2threads ubuntu CI OOM failure
3 participants