Skip to content

buildPythonPackage: Add support for running tests in a separate derivation#272177

Closed
adisbladis wants to merge 2 commits intoNixOS:masterfrom
adisbladis:python-separate-checks
Closed

buildPythonPackage: Add support for running tests in a separate derivation#272177
adisbladis wants to merge 2 commits intoNixOS:masterfrom
adisbladis:python-separate-checks

Conversation

@adisbladis
Copy link
Member

@adisbladis adisbladis commented Dec 5, 2023 •
edited
Loading

Description of changes

Introduces a new argument to buildPythonPackage called separateChecks.

A passthru.tests.python attribute is added where the tests are run separately from building the package.

By removing check inputs from the main derivation we can drastically reduce build closures & reduce issues with circular dependencies.
For more motivations see the related tracking issue.

This work is a part of #272178.

Implications

With tests no longer running in the same derivation as the build it can be harder to find issues early.

OTOH debug cycles can be cut shorter as you don't need to wait for a package rebuild to find out that a test failed because it lacked networking access.
With tests running separately I expect iteration cycles to be quicker, once the new workflow has been adopted.

passthru.tests is evaluated by OfBorg. Checks for nixpkgs contributions should be fine.
It's possible we may want to add another Hydra attrset so it can find tests, or make Hydra smart enough to explore passthru.tests.

In this PR only one derivation has been converted as a PoC. Locally I have converted many more and also found some issues.
This seems to work out fine for most pure Python packages, but Pandas was problematic. I think that particular issue is because of a bug in pytest that I will take a closer look at.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@adisbladis adisbladis requested a review from FRidh as a code owner December 5, 2023 02:08
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Dec 5, 2023
@adisbladis adisbladis mentioned this pull request Dec 5, 2023
Open
3 tasks
@adisbladis adisbladis marked this pull request as draft December 5, 2023 02:24
@adisbladis
Copy link
Member Author

adisbladis commented Dec 5, 2023

I'm marking this as draft: Not because I don't think it's ready for review but because I think we need to have a discussion around the implications it has to remove python test running from the critical path in nixpkgs.

@ofborg ofborg bot requested a review from piegamesde December 5, 2023 06:19
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 5, 2023
@FRidh
Copy link
Member

FRidh commented Dec 5, 2023 •
edited
Loading

passthru.tests is evaluated by OfBorg. Checks for nixpkgs contributions should be fine.
It's possible we may want to add another Hydra attrset so it can find tests, or make Hydra smart enough to explore passthru.tests.

I think it would be useful to introduce a convention such as passthru.tests.package or passthru.tests.source to denote the tests that are part of the source.

@FRidh
Copy link
Member

FRidh commented Dec 5, 2023

I'm marking this as draft: Not because I don't think it's ready for review but because I think we need to have a discussion around the implications it has to remove python test running from the critical path in nixpkgs.

I think if we introduce such a convention, and then update our release* sets so Hydra pick these up that we're good to go.

SuperSandro2000
SuperSandro2000 reviewed Dec 5, 2023
View reviewed changes
SuperSandro2000
SuperSandro2000 reviewed Dec 5, 2023
View reviewed changes
SuperSandro2000
SuperSandro2000 reviewed Dec 5, 2023
View reviewed changes
@adisbladis adisbladis force-pushed the python-separate-checks branch from 92fc039 to 81d5779 Compare February 20, 2024 05:12
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. and removed 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 20, 2024
@adisbladis adisbladis force-pushed the python-separate-checks branch from 81d5779 to aa22c6d Compare February 20, 2024 09:44
SuperSandro2000
SuperSandro2000 reviewed Feb 20, 2024
View reviewed changes
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 5, 2024
This was referenced Jul 11, 2024
Draft
Merged
@adisbladis adisbladis force-pushed the python-separate-checks branch 2 times, most recently from 4534349 to f2f8a1f Compare July 16, 2024 03:10
@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: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jul 16, 2024
@adisbladis adisbladis marked this pull request as ready for review July 16, 2024 04:17
@adisbladis adisbladis requested a review from natsukium as a code owner July 16, 2024 04:17
@adisbladis
Copy link
Member Author

adisbladis commented Jul 16, 2024

Rebased & marked ready for review.

@adisbladis adisbladis force-pushed the python-separate-checks branch 2 times, most recently from ced195f to 936fe14 Compare July 16, 2024 06:44
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. and removed 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. labels Jul 16, 2024
@adisbladis adisbladis force-pushed the python-separate-checks branch from 936fe14 to 37fe155 Compare July 16, 2024 09:30
@adisbladis adisbladis force-pushed the python-separate-checks branch from 37fe155 to b315c18 Compare July 16, 2024 09:38
@ofborg ofborg bot added 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. and removed 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jul 16, 2024
yajo
yajo approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@yajo yajo left a comment

Choose a reason for hiding this comment

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

I love it!

@ShamrockLee
Copy link
Contributor

ShamrockLee commented Aug 17, 2024

Considering the number of rebuilding packages, should this PR target the staging branch instead?

@dotlambda
Copy link
Member

dotlambda commented Sep 1, 2024

One thing I like about packageOverrides is that all tests are run again, which should ensure intercompatibility of the pinned versions. We'd lose this if a package doesn't depend on its test (and if we get rid of packageOverrides).

@adisbladis
Copy link
Member Author

adisbladis commented Sep 2, 2024

One thing I like about packageOverrides is that all tests are run again, which should ensure intercompatibility of the pinned versions. We'd lose this if a package doesn't depend on its test (and if we get rid of packageOverrides).

This is what I meant by:

I think we need to have a discussion around the implications it has to remove python test running from the critical path in nixpkgs.

I'm not sure what the fix for that is.
One thing I was considering to get some of those benefits is to push the responsibility of aggregating tests to buildPythonApplication/toPythonApplication (together with #272179), though that would only run tests for libraries depended on by applications, not loose libraries.

@yajo
Copy link
Contributor

yajo commented Sep 2, 2024

My suggestion is that the derivation produced by this new implementation adds automatically some passtrhu.test attribute or similar. This way we can simply run those tests if available, but still have no direct dependency over them. If we depend on them, then there's no gain: we'd need to rebuild always too.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one person. 2.status: merge conflict This PR has merge conflicts with the target branch labels Sep 8, 2024
@adisbladis adisbladis closed this Nov 11, 2024
@ShamrockLee
Copy link
Contributor

ShamrockLee commented Dec 10, 2024

@adisbladis, would there be a chance to learn about the considerations behind closing this PR? I look forward to it, and many people approve of the idea.

@adisbladis
Copy link
Member Author

adisbladis commented Dec 11, 2024

@adisbladis, would there be a chance to learn about the considerations behind closing this PR? I look forward to it, and many people approve of the idea.

I have burnt out on trying to accomplish meaningful change in the nixpkgs Python infra and I'm spending all that energy towards pyproject.nix & it's builders instead.

@wolfgangwalther wolfgangwalther mentioned this pull request Jan 23, 2025
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@FRidh FRidh Awaiting requested review from FRidh

@piegamesde piegamesde Awaiting requested review from piegamesde

@natsukium natsukium Awaiting requested review from natsukium

+1 more reviewer

@yajo yajo yajo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally 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: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@adisbladis @FRidh @ShamrockLee @dotlambda @yajo @SuperSandro2000 @natsukium @wegank