-
Notifications
You must be signed in to change notification settings - Fork 659
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
Limit threads usage in numpy during test to avoid time-out #4584
base: develop
Are you sure you want to change the base?
Changes from 9 commits
97df4a4
21435cf
12adcb1
06f8d34
33ba400
757d773
e3192e7
747bfb8
e46b809
754ff2c
555520a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,13 @@ jobs: | |
- name: run_tests | ||
if: contains(matrix.name, 'asv_check') != true | ||
run: | | ||
export OPENBLAS_NUM_THREADS=1 | ||
export GOTO_NUM_THREADS=1 | ||
export OMP_NUM_THREADS=1 | ||
export MKL_NUM_THREADS=1 | ||
# limit to 2 workers to avoid overloading the CI | ||
export PYTEST_XDIST_AUTO_NUM_WORKERS=2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be set to auto, because that's what we are already using. Does auto not work correctly? If auto does not work then I'd prefer we define a variable GITHUB_CI_MAXCORES or similar and then use it invoking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was planning to find a way to find the default number of workers pytest will use and reduce it by one because the Ubuntu runner has 4 cores and Mac has 3 cores. but I ended up setting it to 2 and found the performance acceptable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's unclear from this conversation that altering the value of If it's not affecting things, then my suggestion is to stick with auto unless there's a substantial difference in performance that we've missed. |
||
|
||
PYTEST_FLAGS="--disable-pytest-warnings --durations=50" | ||
if [ ${{ matrix.codecov }} = "true" ]; then | ||
PYTEST_FLAGS="${PYTEST_FLAGS} --cov-config=.coveragerc --cov=MDAnalysis --cov-report=xml" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,6 +128,13 @@ jobs: | |
displayName: 'Check installed packages' | ||
- powershell: | | ||
cd testsuite | ||
$env:OPENBLAS_NUM_THREADS=1 | ||
$env:GOTO_NUM_THREADS=1 | ||
$env:OMP_NUM_THREADS=1 | ||
$env:MKL_NUM_THREADS=1 | ||
# limit to 2 workers to avoid overloading the CI | ||
$env:PYTEST_XDIST_AUTO_NUM_WORKERS=1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment is confusing: if you set workers to 1 but say you limit to 2 workers. Change comment, otherwise same as above: perhaps just add to commandline args? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to correct the comment line when I realized Azure has only 2 cores so I changed # workers to 1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above, is this actually affecting timeouts or is this some separate optimization? |
||
|
||
pytest MDAnalysisTests --disable-pytest-warnings -n auto --timeout=200 -rsx --cov=MDAnalysis | ||
orbeckst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
displayName: 'Run MDAnalysis Test Suite' | ||
- script: | | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: there's another place to put this so it applies everywhere