mdanalysis icon indicating copy to clipboard operation
mdanalysis copied to clipboard

Limit threads usage in numpy during test to avoid time-out

Open yuxuanzhuang opened this issue 9 months ago • 8 comments

fix #4209

Changes made in this Pull Request:

  • make numpy only use one thread during test
  • reduce pytest worker
  • make tests lighter

Similar to https://github.com/MDAnalysis/mdanalysis/pull/2950, allowing numpy to make full usages of the resources clogs the machine and likely leads to the ~~occassional~~ frequent time-out in multiprocessing-related testings.

See the comparison between duration below; it takes a lot more time in test_encore for numpy calculations before.

new test duration

============================= slowest 50 durations =============================
11.82s call     MDAnalysisTests/analysis/test_contacts.py::TestContacts::test_n_initial_contacts[datafiles1-41814]
10.93s call     MDAnalysisTests/coordinates/test_gro.py::TestGROLargeWriter::test_writer_large
10.70s call     MDAnalysisTests/coordinates/test_pdb.py::test_conect_bonds_all
9.34s call     MDAnalysisTests/parallelism/test_multiprocessing.py::test_multiprocess_COG[u11]
9.02s call     MDAnalysisTests/utils/test_pickleio.py::test_fileio_pickle
8.53s call     MDAnalysisTests/analysis/test_gnm.py::test_closeContactGNMAnalysis
8.31s call     MDAnalysisTests/analysis/test_gnm.py::test_closeContactGNMAnalysis_weights_None
8.27s call     MDAnalysisTests/analysis/test_encore.py::TestEncore::test_ces_error_estimation_ensemble_bootstrap
7.62s call     MDAnalysisTests/coordinates/test_gro.py::TestGROLargeWriter::test_write_trajectory_universe
7.52s call     MDAnalysisTests/coordinates/test_gro.py::TestGROLargeWriter::test_write_trajectory_atomgroup

old test duration (https://github.com/MDAnalysis/mdanalysis/actions/runs/8869719078/job/24350703650)

20.06s call     MDAnalysisTests/analysis/test_encore.py::TestEncore::test_hes_to_self
18.93s call     MDAnalysisTests/analysis/test_encore.py::TestEncore::test_hes
13.07s call     MDAnalysisTests/analysis/test_encore.py::TestEncore::test_ces_error_estimation_ensemble_bootstrap
12.93s call     MDAnalysisTests/analysis/test_encore.py::TestEncore::test_hes_custom_weights
12.80s call     MDAnalysisTests/coordinates/test_pdb.py::test_conect_bonds_all
12.12s call     MDAnalysisTests/analysis/test_contacts.py::TestContacts::test_n_initial_contacts[datafiles1-41814]
11.40s call     MDAnalysisTests/analysis/test_gnm.py::test_closeContactGNMAnalysis
10.79s call     MDAnalysisTests/coordinates/test_gro.py::TestGROLargeWriter::test_writer_large

gonna run it three times.

pass once image

pass twice image

PR Checklist

  • [ ] Tests?
  • [ ] Docs?
  • [ ] CHANGELOG updated?
  • [ ] Issue raised/referenced?

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4584.org.readthedocs.build/en/4584/

yuxuanzhuang avatar May 01 '24 05:05 yuxuanzhuang

Linter Bot Results:

Hi @yuxuanzhuang! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location Outcome
main package ✅ Passed
testsuite ⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/10782714307/job/29903235743


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

github-actions[bot] avatar May 01 '24 05:05 github-actions[bot]

Codecov Report

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

Project coverage is 93.84%. Comparing base (b3208b3) to head (555520a).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4584      +/-   ##
===========================================
- Coverage    93.87%   93.84%   -0.04%     
===========================================
  Files          173      185      +12     
  Lines        21428    22494    +1066     
  Branches      3980     3980              
===========================================
+ Hits         20116    21109     +993     
- Misses         858      931      +73     
  Partials       454      454              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 01 '24 05:05 codecov[bot]

Hello @yuxuanzhuang! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 128:53: E231 missing whitespace after ','

Comment last updated at 2024-09-09 23:10:45 UTC

pep8speaks avatar May 01 '24 06:05 pep8speaks

@orbeckst @IAlibay I now believe that these three adjustments can reduce the chance of timeouts:

  1. Limiting the number of threads used by numpy during testing. This also decreases the duration of tests that are heavily dependent on numpy and computationally intensive, e.g. in encore.

  2. Reducing the number of workers in pytest to prevent overloading the entire test runner.

  3. Lightening the tests. I have already modified some, but we can certainly further optimize the other time-consuming tests.

I have re-run the GitHub CI three times consecutively, and they all pass. I have also patched it to #4162 that are more likely to time-out in https://github.com/yuxuanzhuang/mdanalysis/pull/6 and it also seems to work well.

yuxuanzhuang avatar May 03 '24 16:05 yuxuanzhuang

@IAlibay would you please shepherd the PR to completion?

orbeckst avatar May 03 '24 22:05 orbeckst

@IAlibay with my PR shepherding hat on, just pinging here, no rush though :smile:

hmacdope avatar May 27 '24 06:05 hmacdope

@IAlibay just pinging here with my review coordination hat on.

hmacdope avatar Jul 20 '24 02:07 hmacdope

Since this PR doesn’t really resolve the time-out issue, I will repurpose it to focus on reducing the test duration.

yuxuanzhuang avatar Sep 09 '24 23:09 yuxuanzhuang