Hello Pythonistats, packagers,
A handful of Fedora Python packages uses pytest-xdist to run tests in parallel like this:
%pytest -n auto
-n auto means pytest will spawn a number of workers processes equal to the number of available CPUs.
In the spirit of other packaging guidelines, I believe we should use this instead:
%pytest -n %{_smp_build_ncpus}
This means the same thing in most of the ceases, but will limit the number of workers depending on other constraints in the spec or in the environment.
Should I do this in a mass change? Not so many packages use pytest -n auto in the spec:
$ rg -l -- '(-n|--numprocesses)(\s*|=)auto(\s|$)' ansible-bender.spec azure-cli.spec ocrmypdf.spec python-cartopy.spec python-GridDataFormats.spec python-hypothesis.spec python-matplotlib.spec python-mplcairo.spec python-rpmautospec.spec python-sqlalchemy.spec python-tox.spec python-xarray.spec python-zstandard.spec pythran.spec scipy.spec
(Other packages have that in tox.ini or pytest config file, but I am not aiming at changing that here.)
And, considering many other packages might want to benefit from that, should this be:
1) encouraged in the Python packaging guidelines 2) macronized (I was thinking %pytest_parallel, but TBD)
?
On 07. 07. 22 11:38, Miro Hrončok wrote:
Hello Pythonistats, packagers,
A handful of Fedora Python packages uses pytest-xdist to run tests in parallel like this:
%pytest -n auto
-n auto means pytest will spawn a number of workers processes equal to the number of available CPUs.
In the spirit of other packaging guidelines, I believe we should use this instead:
%pytest -n %{_smp_build_ncpus}
This means the same thing in most of the ceases, but will limit the number of workers depending on other constraints in the spec or in the environment.
Should I do this in a mass change? Not so many packages use pytest -n auto in the spec:
$ rg -l -- '(-n|--numprocesses)(\s*|=)auto(\s|$)' ansible-bender.spec azure-cli.spec ocrmypdf.spec python-cartopy.spec python-GridDataFormats.spec python-hypothesis.spec python-matplotlib.spec python-mplcairo.spec python-rpmautospec.spec python-sqlalchemy.spec python-tox.spec python-xarray.spec python-zstandard.spec pythran.spec scipy.spec
(Other packages have that in tox.ini or pytest config file, but I am not aiming at changing that here.)
And, considering many other packages might want to benefit from that, should this be:
1) encouraged in the Python packaging guidelines 2) macronized (I was thinking %pytest_parallel, but TBD)
Or pytest-xdist could be taught to check an environment variable for `auto`, making this seamless for packagers?
On 07. 07. 22 14:00, Petr Viktorin wrote:
On 07. 07. 22 11:38, Miro Hrončok wrote:
Hello Pythonistats, packagers,
A handful of Fedora Python packages uses pytest-xdist to run tests in parallel like this:
%pytest -n auto
-n auto means pytest will spawn a number of workers processes equal to the number of available CPUs.
In the spirit of other packaging guidelines, I believe we should use this instead:
%pytest -n %{_smp_build_ncpus}
This means the same thing in most of the ceases, but will limit the number of workers depending on other constraints in the spec or in the environment.
Should I do this in a mass change? Not so many packages use pytest -n auto in the spec:
$ rg -l -- '(-n|--numprocesses)(\s*|=)auto(\s|$)' ansible-bender.spec azure-cli.spec ocrmypdf.spec python-cartopy.spec python-GridDataFormats.spec python-hypothesis.spec python-matplotlib.spec python-mplcairo.spec python-rpmautospec.spec python-sqlalchemy.spec python-tox.spec python-xarray.spec python-zstandard.spec pythran.spec scipy.spec
(Other packages have that in tox.ini or pytest config file, but I am not aiming at changing that here.)
And, considering many other packages might want to benefit from that, should this be:
1) encouraged in the Python packaging guidelines 2) macronized (I was thinking %pytest_parallel, but TBD)
Or pytest-xdist could be taught to check an environment variable for `auto`, making this seamless for packagers?
This could work, except that sometimes pytest-xdist is installed and we don't want to run tests in parallel because they are not prepared for that. But I guess an opt-out would still exist, e.g. setting %{_smp_build_ncpus} to 1 in the %check section.
I will have a look if there is an environment variable we could use. We can probably adjust PYTEST_ADDOPTS if we detect pytest-xdist is available.
On 07. 07. 22 14:44, Miro Hrončok wrote:
On 07. 07. 22 14:00, Petr Viktorin wrote:
On 07. 07. 22 11:38, Miro Hrončok wrote:
Hello Pythonistats, packagers,
A handful of Fedora Python packages uses pytest-xdist to run tests in parallel like this:
%pytest -n auto
-n auto means pytest will spawn a number of workers processes equal to the number of available CPUs.
In the spirit of other packaging guidelines, I believe we should use this instead:
%pytest -n %{_smp_build_ncpus}
This means the same thing in most of the ceases, but will limit the number of workers depending on other constraints in the spec or in the environment.
Should I do this in a mass change? Not so many packages use pytest -n auto in the spec:
$ rg -l -- '(-n|--numprocesses)(\s*|=)auto(\s|$)' ansible-bender.spec azure-cli.spec ocrmypdf.spec python-cartopy.spec python-GridDataFormats.spec python-hypothesis.spec python-matplotlib.spec python-mplcairo.spec python-rpmautospec.spec python-sqlalchemy.spec python-tox.spec python-xarray.spec python-zstandard.spec pythran.spec scipy.spec
(Other packages have that in tox.ini or pytest config file, but I am not aiming at changing that here.)
And, considering many other packages might want to benefit from that, should this be:
1) encouraged in the Python packaging guidelines 2) macronized (I was thinking %pytest_parallel, but TBD)
Or pytest-xdist could be taught to check an environment variable for `auto`, making this seamless for packagers?
This could work, except that sometimes pytest-xdist is installed and we don't want to run tests in parallel because they are not prepared for that. But I guess an opt-out would still exist, e.g. setting %{_smp_build_ncpus} to 1 in the %check section.
I will have a look if there is an environment variable we could use. We can probably adjust PYTEST_ADDOPTS if we detect pytest-xdist is available.
I meant teaching pytest-xdist to look at an envvar, which would just be ignored if xdist isn't around. Actually I just went ahead and proposed it upstream: https://github.com/pytest-dev/pytest-xdist/issues/792
On 12. 07. 22 11:25, Petr Viktorin wrote:
Or pytest-xdist could be taught to check an environment variable for `auto`, making this seamless for packagers?
This could work, except that sometimes pytest-xdist is installed and we don't want to run tests in parallel because they are not prepared for that. But I guess an opt-out would still exist, e.g. setting %{_smp_build_ncpus} to 1 in the %check section.
I will have a look if there is an environment variable we could use. We can probably adjust PYTEST_ADDOPTS if we detect pytest-xdist is available.
I meant teaching pytest-xdist to look at an envvar, which would just be ignored if xdist isn't around. Actually I just went ahead and proposed it upstream: https://github.com/pytest-dev/pytest-xdist/issues/792
Oh, now I get it. Perfect!
On 12. 07. 22 11:50, Miro Hrončok wrote:
On 12. 07. 22 11:25, Petr Viktorin wrote:
Or pytest-xdist could be taught to check an environment variable for `auto`, making this seamless for packagers?
This could work, except that sometimes pytest-xdist is installed and we don't want to run tests in parallel because they are not prepared for that. But I guess an opt-out would still exist, e.g. setting %{_smp_build_ncpus} to 1 in the %check section.
I will have a look if there is an environment variable we could use. We can probably adjust PYTEST_ADDOPTS if we detect pytest-xdist is available.
I meant teaching pytest-xdist to look at an envvar, which would just be ignored if xdist isn't around. Actually I just went ahead and proposed it upstream: https://github.com/pytest-dev/pytest-xdist/issues/792
Oh, now I get it. Perfect!
A followup:
Petr's change was merged to pytest-xdsit:
https://github.com/pytest-dev/pytest-xdist/pull/829
I've drafted a change for %pytest:
https://src.fedoraproject.org/rpms/python-rpm-macros/pull-request/149
Change for %tox will follow.
Packages that call pytest directly (i.e. not by the macros) won't benefit from this change.
On 7/7/22 04:38, Miro Hrončok wrote:
Should I do this in a mass change? Not so many packages use pytest -n auto in the spec:
$ rg -l -- '(-n|--numprocesses)(\s*|=)auto(\s|$)' azure-cli.spec
As the azure-cli maintainer, I'm okay with that change.
(Other packages have that in tox.ini or pytest config file, but I am not aiming at changing that here.)
And, considering many other packages might want to benefit from that, should this be:
1) encouraged in the Python packaging guidelines
Of course! I always forget the "%{_smp_build_ncpus}" macro and end up digging through macros to find how to spell it.
2) macronized (I was thinking %pytest_parallel, but TBD)
That's a good idea, but I don't know how much use it would get. 🤷🏻♂️
On 7/7/22 05:38, Miro Hrončok wrote:
In the spirit of other packaging guidelines, I believe we should use this instead:
%pytest -n %{_smp_build_ncpus}
I agree that this is more strictly correct.
Should I do this in a mass change? Not so many packages use pytest -n auto in the spec:
I think this would be reasonable.
And, considering many other packages might want to benefit from that, should this be:
1) encouraged in the Python packaging guidelines
I think it would be great to at least document how to do it in the guidelines. I had a vague sense that pytest-xdist allowed parallelization, but I didn’t know off the top of my head that it added a straightforward pytest option.
I’m torn on whether we should make this a SHOULD, and on whether we should try to automate it, expecting packagers to opt out as needed. On one hand, both would be consistent with the existing guidelines for Make[1]. On the other hand, I think it may be even more common in Python-land to encounter test suites that aren’t parallel-safe due to things like filesystem race conditions, and these issues can be difficult to diagnose and debug if one isn’t already looking for them.
Where upstream already pulls in pytest-xdist and uses “-n auto” or similar—but perhaps in a tox.ini, shell script, CI configuration, Makefile, etc. that isn’t used when running the tests for the RPM—it’s very safe to parallelize the tests in the RPM build, and a good idea to do so.
A good example of the ideal case and potential benefit is python-aws-sam-translator. It has a huge test suite; upstream brings in pytest-xdist via the “dev” extra; and there is a Makefile (not suitable for use in the RPM build) that uses “-n auto”, so I know the tests are safe to parallelize. Adding “-n %{_smp_build_ncpus}” to “%pytest” speeds up the tests from 130 seconds to 40 seconds on a 16-core machine.
2) macronized (I was thinking %pytest_parallel, but TBD)
I’m not as certain about this; it usefully hides the extra option to pytest, but the abstraction leaks because the packager still has to understand that pytest-xdist is responsible and add the necessary BR (or verify that it is already generated).
[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_parallel_make
– Ben
python-devel@lists.fedoraproject.org
-
Ben Beasley -
Major Hayden -
Miro Hrončok -
Petr Viktorin