pace-neutrons / Euphonic

Euphonic is a Python package for efficient simulation of phonon bandstructures, density of states and inelastic neutron scattering intensities from force constants.
GNU General Public License v3.0
29 stars 11 forks source link

run_tests workflow: use parallel tox #322

Open ajjackson opened 3 weeks ago

ajjackson commented 3 weeks ago

Alternative to #321 : parallelism at the tox level

Let's see if we get a more impressive speedup by distributing the tox environments over separate processors.

Depends on #324

github-actions[bot] commented 3 weeks ago

Test Results

    30 files  +    8      30 suites  +8   2h 27m 59s ⏱️ + 1h 58m 21s  1 061 tests ±    0   1 055 ✅ ±    0   6 💤 ± 0  0 ❌ ±0  14 770 runs  +4 220  14 682 ✅ +4 195  88 💤 +25  0 ❌ ±0 

Results for commit 369635c4. ± Comparison against base commit 23fb80b9.

:recycle: This comment has been updated with latest results.

ajjackson commented 3 weeks ago

Well, in the initial naive attempt the Mac performance is great (5mins) and the Win/Linux runs have build problems, probably related to race conditions while the parallel process build Euphonic binaries.

ajjackson commented 3 weeks ago

I suspect that migration to pyproject.toml and tox 4 would solve this problem by better isolating the builds. But a workaround to try before then is to build the tox environments in series and then run them in parallel.

ajjackson commented 3 weeks ago

This is odd, Windows is failing to build even in series now?

   D:\a\Euphonic\Euphonic\c\_euphonic.c : fatal error C1083: Cannot open compiler generated file: 'D:\a\Euphonic\Euphonic\build\temp.win-amd64-cpython-310\Release\c\_euphonic.obj': Permission denied
  error: command 'C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Tools\\MSVC\\14.41.34120\\bin\\HostX86\\x64\\cl.exe' failed with exit code 1
ajjackson commented 3 weeks ago

The problem is that Euphonic is installed in pre_command, not install_command, and pre_command doesn't run under --notest. This is done to ensure Numpy is available before Euphonic is installed, so the solution is not as simple as adding euphonic to the install_command requirements.

ajjackson commented 3 weeks ago

I'm seeing some elaborate workaround options, but propose that this is put on hold until the project is migrated to a better-isolated build process with pyproject.toml, and the test-env framework migrated to tox 4. It may be that under those conditions all we need to do is add --parallel.

@oerc0122 has taken an initial look at migration to pyproject.toml and hit a few (likely surmountable) problems, so we should assume it will take some effort to finish that.

ajjackson commented 4 days ago

Have tried reimplementing this on top of #324

ajjackson commented 4 days ago

One Windows test failure; a few values in an array on Python 3.12. Not sure how to attack that 😞

oerc0122 commented 4 days ago

Is it due to list order? Or is it actually a failure?

Are the tests wholly independent or are they writing to files. Temp files struggle on Windows systems due to how Windows resolves and updates its filesystem, could that be an issue?

ajjackson commented 4 days ago

It's an odd one. Not very file-related. This is mostly a test that gamma-splits will be weighted appropriately, but also that the act of weighting doesn't change the frequencies. Here it does! They are frequencies not eigenvectors so in the case of degeneracy an order change shouldn't matter.

================================== FAILURES ===================================
_ TestForceConstantsCalculateQPointPhononModes.test_calculate_qpoint_phonon_modes_with_weights_doesnt_change_result[fc1-qpts1-weights1-expected_weights1-kwargs1] _

self = <tests_and_analysis.test.euphonic_test.test_force_constants_calculate_qpoint_phonon_modes.TestForceConstantsCalculateQPointPhononModes object at 0x0000026E2FD91C10>
fc = <euphonic.force_constants.ForceConstants object at 0x0000026E2FD42F90>
qpts = array([[ 1.      ,  1.      ,  1.      ],
       [ 0.      ,  0.      ,  0.5     ],
       [ 0.      ,  0.      ,  0. ...[-1.      ,  1.      ,  1.      ],
       [-0.151515,  0.575758,  0.5     ],
       [-1.      ,  1.      ,  1.      ]])
weights = array([0.1 , 0.05, 0.05, 0.2 , 0.2 , 0.15, 0.15, 0.2 , 0.1 ])
expected_weights = array([0.1  , 0.05 , 0.025, 0.025, 0.2  , 0.1  , 0.1  , 0.075, 0.075,
       0.075, 0.075, 0.2  , 0.1  ])
kwargs = {'insert_gamma': True}

    @pytest.mark.parametrize('fc, qpts, weights, expected_weights, kwargs', [
        (get_fc('quartz'), get_test_qpts(), weights, weights, {}),
        (get_fc('quartz'), get_test_qpts('split_insert_gamma'), weights,
         weights_output_split_gamma, {'insert_gamma': True})])
    def test_calculate_qpoint_phonon_modes_with_weights_doesnt_change_result(
            self, fc, qpts, weights, expected_weights, kwargs):
        qpt_ph_modes_weighted = fc.calculate_qpoint_phonon_modes(
            qpts, weights=weights, **kwargs)
        qpt_ph_modes_unweighted = fc.calculate_qpoint_phonon_modes(
            qpts, **kwargs)
        qpt_ph_modes_unweighted.weights = expected_weights
>       check_qpt_ph_modes(qpt_ph_modes_weighted, qpt_ph_modes_unweighted)

euphonic_test\test_force_constants_calculate_qpoint_phonon_modes.py:259: 
ajjackson commented 3 days ago

Oh no, it passed

ajjackson commented 3 days ago

Windows also looking happy now that I've re-run including Python 3.11