sbi-dev / sbi

Simulation-based inference toolkit
https://sbi-dev.github.io/sbi/
Apache License 2.0
578 stars 145 forks source link

CPU Saturation test (prerequisite for #1175 PR) #1187

Closed janko-petkovic closed 2 months ago

janko-petkovic commented 3 months ago

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.

codecov[bot] commented 3 months ago

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 ```diff @@ 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](https://app.codecov.io/gh/sbi-dev/sbi/pull/1187/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/sbi-dev/sbi/pull/1187/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev) | `75.61% <ø> (-8.95%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev#carryforward-flags-in-the-pull-request-comment) to find out more. [see 24 files with indirect coverage changes](https://app.codecov.io/gh/sbi-dev/sbi/pull/1187/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sbi-dev)
janfb commented 2 months ago

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?

janko-petkovic commented 2 months ago

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 commented 2 months ago

Here are the performances before and after the simulate_for_sbi refactoring:

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

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).