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

CircleCI pipeline optimisations #138

Merged
merged 21 commits into from
Dec 1, 2021
Merged

Conversation

zmarkan
Copy link

@zmarkan zmarkan commented Dec 1, 2021

Proposed changes

Optimisations to the CircleCI pipeline:

  • split the single build & test across 3 jobs which run in parallel
  • use CircleCI test splitting to further parallelise tests in test_parametric_components (2x) and test_parametric_reactors (4x)
  • Use large resource class in test_parametric_reactors to utilise 4x virtual CPUs instead of 2
  • refactor repeated code snippets for docker image executor and commands for installing dependencies and codecov upload

Types of changes

What types of changes does your code introduce to the Paramak?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code refactoring
  • Documentation Update (if none of the other choices apply)
  • New tests

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • Pep8 applied
  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

shimwell and others added 21 commits July 6, 2021 08:10
Adding Cubit to Dockerfile and making moab and jupyter_cadquery optional
refactor and removal of redundant neutronics/h5m related code
merged breps, improved stl export, default parameters for reactors
Adding system code model of FLF reactor and typos fixing
add another test suite

add parametric rectors tests

try test splitting

add store tests results

parallelism for parametric components tests
@zmarkan
Copy link
Author

zmarkan commented Dec 1, 2021

Here's some docs on test splitting and parallelisation: https://circleci.com/docs/2.0/parallelism-faster-jobs/

@zmarkan
Copy link
Author

zmarkan commented Dec 1, 2021

I also played around with the branch that was timing out often because of the 1hr job duration limit: https://app.circleci.com/pipelines/github/zmarkan/paramak?branch=negative_triangularity_MC

With the new test splitting in place it shouldn't break, but if you can refactor the tests into multiple files (where it makes sense of course) it could go much faster still. The longest running test is by far in test_negative_triangularity_reactor.py in that negative triangularity branch.

@shimwell
Copy link
Member

shimwell commented Dec 1, 2021

@RemDelaporteMathurin I think you might like this PR, I meet with CircleCI pro @zmarkan and he has some nice tricks for splitting up the tests which should speed it up considerably.

The magic lines include parallelism: 2 and a different way of running pytest command that provides a list of separate files.

TEST_FILES=$(circleci tests glob "tests/test_parametric_components/**/*.py" | circleci tests split)
pytest -v --cov=paramak --cov-append --cov-report term --cov-report xml --junitxml=test-reports/junit.xml $TEST_FILES

@shimwell
Copy link
Member

shimwell commented Dec 1, 2021

Previously all tests ran in serial and took between 16m and 1 hour to run and would sometimes time out

Now the tests finish in 3 mins as they run in parallel

Here is a break down of the timings

parametric-components-tests times were 2:30 and 2.08 mins
parametric-reactors-tests were 3:27, 2:26, 2:34, 1:17, 3:05 and 0:49
quick-tests runs in 1:05

@codecov
Copy link

codecov bot commented Dec 1, 2021

Codecov Report

Merging #138 (ecf9a46) into main (bee4f04) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #138   +/-   ##
=======================================
  Coverage   97.56%   97.56%           
=======================================
  Files          76       76           
  Lines        4717     4717           
=======================================
  Hits         4602     4602           
  Misses        115      115           

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 bee4f04...ecf9a46. Read the comment docs.

@shimwell
Copy link
Member

shimwell commented Dec 1, 2021

This is super stuff, @zmarkan

We have been trying to add everyone who helps to the CITATIONS.md file, would you be happy if we add you there?

@shimwell
Copy link
Member

shimwell commented Dec 1, 2021

I also played around with the branch that was timing out often because of the 1hr job duration limit: https://app.circleci.com/pipelines/github/zmarkan/paramak?branch=negative_triangularity_MC

With the new test splitting in place it shouldn't break, but if you can refactor the tests into multiple files (where it makes sense of course) it could go much faster still. The longest running test is by far in test_negative_triangularity_reactor.py in that negative triangularity branch.

@mateczentye looks like your new reactor is the most complex one so far :-)

@zmarkan
Copy link
Author

zmarkan commented Dec 1, 2021

@shimwell thank you! I'm glad it's useful - yes please I'd be happy to be added to CITATIONS.md!

@shimwell shimwell changed the base branch from main to develop December 1, 2021 21:15
@shimwell shimwell merged commit 1dc670a into fusion-energy:develop Dec 1, 2021
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.

2 participants