Skip to content

Conversation

@Artturin
Copy link
Member

@Artturin Artturin commented Mar 30, 2022

many of our packages have pytest-xdist in their inputs however
oftentimes the required flags aren't added to pytest flags
since we dont use tox and other frameworks

some examples of improvements
(my pc has ryzen 9 5900x)
hypothesis 11m:15s on hydra -> <1m on my pc
hypothesmith 8m on my pc -> <1m on my pc

the whole graph from pytest-xdist (tested by adding postPatch with true to xdist)
time nix build ".#python3Packages.hypothesmith"
19m:57s -> 9m:13s

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Mar 30, 2022
@ofborg ofborg bot requested a review from dotlambda March 30, 2022 22:48
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Mar 30, 2022
@FRidh
Copy link
Member

FRidh commented Mar 31, 2022

Not relevant right now, but in case checkInputs gets split into a checkNativeInputs and checkInputs we may need to have this library in both in order to have the hook run. Though I am not sure about that.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I think this will be very valuable. Thank you!

@roberth roberth removed their request for review March 31, 2022 09:08
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this duplicate what happens in the setup hook?

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't think the setup hook is used at this point yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

just tested and yep its not used at this point

if i apply the following the build fails with
python3.9-pytest-xdist> substitute(): ERROR: file '/nix/store/q4qj7q82vrxfwnmymyhdwxid0sr0rjnn-hook' does not exist

diff --git a/pkgs/development/python-modules/pytest-xdist/default.nix b/pkgs/development/python-modules/pytest-xdist/default.nix
index 75fd066c76d..ae23bf3f31b 100644
--- a/pkgs/development/python-modules/pytest-xdist/default.nix
+++ b/pkgs/development/python-modules/pytest-xdist/default.nix
@@ -1,4 +1,5 @@
 { lib
+, makeSetupHook
 , buildPythonPackage
 , fetchPypi
 , pythonOlder
@@ -12,6 +13,10 @@
 , pexpect
 }:
 
+let
+  setupHook' = makeSetupHook {} ./setup-hook.sh;
+in
+
 buildPythonPackage rec {
   pname = "pytest-xdist";
   version = "2.5.0";
@@ -26,11 +31,9 @@ buildPythonPackage rec {
   buildInputs = [
     pytest
   ];
-  checkInputs = [ pytestCheckHook filelock pexpect ];
+  checkInputs = [ pytestCheckHook filelock pexpect setupHook' ];
   propagatedBuildInputs = [ execnet pytest-forked psutil ];
 
-  pytestFlagsArray = [ "-n $NIX_BUILD_CORES" "--forked" ]; # pytest can already use xdist at this point
-
   # access file system
   disabledTests = [
     "test_distribution_rsyncdirs_example"
@@ -43,7 +46,7 @@ buildPythonPackage rec {
     "test_internal_errors_propagate_to_controller"
   ];
 
-  setupHook = ./setup-hook.sh;
+  setupHook = setupHook';
 
   meta = with lib; {
     description = "Pytest xdist plugin for distributed testing and loop-on-failing modes";

@mweinelt
Copy link
Member

I think this is a reasonable change and we can test it in #166489 if you rebase onto that.

@Artturin Artturin requested a review from Ericson2314 March 31, 2022 13:26
many of our packages have pytest-xdist in their inputs however
oftentimes the required flags aren't added to pytest flags
since we dont use tox and other frameworks

some examples of improvements
(my pc has ryzen 9 5900x)
hypothesis 11m:15s on hydra -> <1m on my pc
hypothesmith 8m on my pc -> <1m on my pc

the whole graph from pytest-xdist (tested by adding postPatch with true to xdist)
time nix build ".#python3Packages.hypothesmith"
19m:57s -> 9m:13s
@Artturin Artturin changed the base branch from staging to python-updates March 31, 2022 13:38
@roberth
Copy link
Member

roberth commented Mar 31, 2022

I don't usually package python, so I'll unsubscribe after my superficial observation earlier.

@mweinelt mweinelt merged commit e8c63a0 into NixOS:python-updates Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants