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

[MRG] Parallelize pytests where applicable #932

Merged
merged 3 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions .github/workflows/unix_unit_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}

- name: Install Python dependencies
shell: bash -el {0}
run: |
if [ ${{ matrix.os }} == 'macos-latest' ]; then
python -m pip install --upgrade pip
conda install --yes -c conda-forge mpi4py openmpi
elif [[ "${{ matrix.os }}" == "ubuntu-latest" ]]; then
elif [[ "${{ matrix.os }}" == "ubuntu-latest" ]]; then
python -m pip install --upgrade pip
python -m pip install mpi4py
fi
Expand All @@ -58,11 +58,15 @@
shell: bash -el {0}
run: |
flake8 --count hnn_core
- name: Test with pytest
- name: Test embarrassingly parallel tests with pytest
shell: bash -el {0}
run: |
python -m pytest ./hnn_core/tests/ -m "not already_parallel" -n auto --cov=hnn_core --cov-report=xml
- name: Test true parallel tests with pytest
shell: bash -el {0}
run: |
python -m pytest ./hnn_core/tests/ --cov=hnn_core --cov-report=xml
python -m pytest ./hnn_core/tests/ -m "already_parallel" --cov=hnn_core --cov-report=xml --cov-append
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should add this to the Makefile: https://github.com/jonescompneurolab/hnn-core/blob/master/Makefile

then you use the same command locally too. E.g.:

$ make already_parallel_tests

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the make test has a dependency on already_parallel_tests and parallel_tests like so:

tests : already_parallel parallel_tests

could you come up with slightly better names so the distinction between the two categories are clear?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about @pytest.mark.uses_mpi? That way it is unique, easy to understand (and grep), but also distinct from the other valid decorator @requires_mpi4py.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I like that!

- name: Upload coverage to Codecov
shell: bash -el {0}
run: |
bash <(curl -s https://codecov.io/bash) -f ./coverage.xml
bash <(curl -s https://codecov.io/bash) -f ./coverage.xml
1 change: 1 addition & 0 deletions hnn_core/tests/test_dipole.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ def test_dipole_simulation():

@requires_mpi4py
@requires_psutil
@pytest.mark.already_parallel
def test_cell_response_backends(run_hnn_core_fixture):
"""Test cell_response outputs across backends."""

Expand Down
1 change: 1 addition & 0 deletions hnn_core/tests/test_extracellular.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ def test_transfer_resistance():

@requires_mpi4py
@requires_psutil
@pytest.mark.already_parallel
def test_extracellular_backends(run_hnn_core_fixture):
"""Test extracellular outputs across backends."""
# calculation of CSD requires >=4 electrode contacts
Expand Down
1 change: 1 addition & 0 deletions hnn_core/tests/test_gui.py
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ def test_gui_init_network(setup_gui):

@requires_mpi4py
@requires_psutil
@pytest.mark.already_parallel
def test_gui_run_simulation_mpi():
"""Test if run button triggers simulation with MPIBackend."""
gui = HNNGUI()
Expand Down
2 changes: 2 additions & 0 deletions hnn_core/tests/test_parallel_backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ def test_gid_assignment():


@pytest.mark.incremental
@pytest.mark.already_parallel
class TestParallelBackends():
dpls_reduced_mpi = None
dpls_reduced_default = None
Expand Down Expand Up @@ -241,6 +242,7 @@ def test_compare_hnn_core(self, run_hnn_core_fixture, backend, n_jobs=1):
# class marked incremental
@requires_mpi4py
@requires_psutil
@pytest.mark.already_parallel
def test_mpi_failure(run_hnn_core_fixture):
"""Test that an MPI failure is handled and messages are printed"""
# this MPI parameter will cause a MPI job to fail
Expand Down
2 changes: 2 additions & 0 deletions pytest.ini
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
[pytest]
markers =
already_parallel: tests that should not be run across multiple cores by pytest
incremental: run tests with prerequisites in incremental order

2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def run(self):
extras = {
'opt': ['scikit-learn'],
'parallel': ['joblib', 'psutil'],
'test': ['flake8', 'pytest', 'pytest-cov', ],
'test': ['flake8', 'pytest', 'pytest-cov', 'pytest-xdist'],
'docs': ['mne', 'nibabel', 'pooch', 'tdqm',
'sphinx', 'sphinx-gallery',
'sphinx_bootstrap_theme', 'sphinx-copybutton',
Expand Down
Loading