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

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

Closed
wants to merge 2 commits into from

Conversation

adisbladis
Copy link
Member

@adisbladis adisbladis commented Dec 5, 2023

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
Copy link
Member Author

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.

@FRidh
Copy link
Member

FRidh commented Dec 5, 2023

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.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 5, 2024
@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+ 10.rebuild-darwin: 501-1000 and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jul 16, 2024
@adisbladis adisbladis force-pushed the python-separate-checks branch from f2f8a1f to c1df366 Compare July 16, 2024 04:12
@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

Rebased & marked ready for review.

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

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

@dotlambda
Copy link
Member

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

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 reputable 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

@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, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants