sbi icon indicating copy to clipboard operation
sbi copied to clipboard

CPU Saturation test (prerequisite for #1175 PR)

Open janko-petkovic opened this issue 1 year ago • 1 comments

This branch includes an additional test aimed at highlighting the CPU usage while running a high number of simulations. ...

Does this close any currently open issues?

This is an addition that aims to give the devs a way to test the fix that will be proposed in the next days for the simulate_for_sbi function

Any relevant code examples, logs, error output, etc?

You don't need to let the test finish, it is just supposed to provide the workload.

Any other comments?

Remember that the testing is actually done by looking at the process manager / htop while the test is running.

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.

  • [x] I have read and understood the contribution guidelines
  • [x] I agree with re-licensing my contribution from AGPLv3 to Apache-2.0.
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] I have reported how long the new tests run and potentially marked them with pytest.mark.slow.
  • [x] New and existing unit tests pass locally with my changes
  • [x] I performed linting and formatting as described in the contribution guidelines
  • [x] I rebased on main (or there are no conflicts with main)
  • [ ] For reviewer: The continuous deployment (CD) workflow are passing.

janko-petkovic avatar Jun 26 '24 16:06 janko-petkovic

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.61%. Comparing base (ba19688) to head (4601b04).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1187      +/-   ##
==========================================
- Coverage   84.55%   75.61%   -8.95%     
==========================================
  Files          96       96              
  Lines        7603     7603              
==========================================
- Hits         6429     5749     -680     
- Misses       1174     1854     +680     
Flag Coverage Δ
unittests 75.61% <ø> (-8.95%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

see 24 files with indirect coverage changes

codecov[bot] avatar Jun 26 '24 16:06 codecov[bot]

I suggest, one the other PR #1188 is merged, we can merge main into this branch to then double check whether CPU saturation has improved with the fixes. If yes, we can actually delete the cpu_saturation_test.py as it is not really a CI test. We only keep the changes to multiprocessing_test.py. Would you agree?

janfb avatar Jul 21 '24 16:07 janfb

I suggest, one the other PR #1188 is merged, we can merge main into this branch to then double check whether CPU saturation has improved with the fixes. If yes, we can actually delete the cpu_saturation_test.py as it is not really a CI test. We only keep the changes to multiprocessing_test.py. Would you agree?

Sure makes sense and keeps main history less cluttered. I'd then post the runtime differences in this thread.

janko-petkovic avatar Jul 22 '24 10:07 janko-petkovic

Here are the performances before and after the simulate_for_sbi refactoring:

  • old implementation tqdm-old
  • new implementation tqdm

Interestingly, the CPU usage is not fully saturated in either case:

  • old implementation htop-old
  • new implementation htop

This could be due to the fact that the test is not computationally intensive, as other, heavier, simulations have shown to fully saturate CPU with the new implementation.

At this point, this branch has served its proof-of-concept purpose and can be deleted without merging (to the best of my understanding).

janko-petkovic avatar Jul 22 '24 12:07 janko-petkovic