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

WIP: buildPythonPackage: perform tests in separate derivation #90240

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Jun 13, 2020

Experiment to see what is needed to perform tests in a separate
derivation.

$ nix-build -A python3.pkgs.pytest.tests.installation

Ideally, I would like a Python build to consist of the following three
builds:

  1. build wheel
  2. install wheel
  3. test installation
    where 3) is optional.

Going there we should probably change the interface significantly,
specifically, we should nomenclature for inputs common to Python users.
All the check related attributes are probably best put in an attribute
set as well, but again, that is a big change in interface.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Experiment to see what is needed to perform tests in a separate
derivation.

    $ nix-build -A python3.pkgs.pytest.tests.installation

Ideally, I would like a Python build to consist of the following three
builds:
1) build wheel
2) install wheel
3) test installation
where 3) is optional.

Going there we should probably change the interface significantly,
specifically, we should nomenclature for inputs common to Python users.
All the check related attributes are probably best put in an attribute
set as well, but again, that is a big change in interface.
@FRidh FRidh requested a review from jonringer as a code owner June 13, 2020 14:44
@@ -163,15 +157,42 @@ let
# Python packages built through cross-compilation are always for the host platform.
disallowedReferences = lib.optionals (python.stdenv.hostPlatform != python.stdenv.buildPlatform) [ python.pythonForBuild ];

passthru = passthru // {
tests.installation = stdenv.mkDerivation {
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe better to include the whole self here stdenv.mkDerivatoin (self // {... })

Comment on lines +170 to +177
nativeBuildInputs = [
self
] ++ lib.optionals (format == "setuptools") [
# Longer-term we should get rid of this and require
# users of this function to set the `installCheckPhase` or
# pass in a hook that sets it.
#setuptoolsCheckHook
] ++ checkInputs ++ propagatedBuildInputs;
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be time to just drop this altogether, already commented it out:

Suggested change
nativeBuildInputs = [
self
] ++ lib.optionals (format == "setuptools") [
# Longer-term we should get rid of this and require
# users of this function to set the `installCheckPhase` or
# pass in a hook that sets it.
#setuptoolsCheckHook
] ++ checkInputs ++ propagatedBuildInputs;
nativeBuildInputs = [
self
] ++ checkInputs ++ propagatedBuildInputs;

@jonringer
Copy link
Contributor

having some tests I think is really beneficial for checking for regressions, and it pairs will with nixpkgs-review which essentially means that if it builds, it's working.

I think having separate derivations for tests might be really useful for something like pytorch, in which the test suite is very resource and time intensive. But, only ofborg is realistically going to run it, maybe if nixpkgs-review supports running the tests passthru will it be more useful. But I think the usefulness of this is limited by how much this will be enforced.

Another idea is to have two checkPhases, one that gets ran with the build, and an optional longer test which gets added to passthru.tests. Something like checkPhase and longCheckPhase ... or something to that effect.

I might also be missing what a separate test derivation is trying to solve.

@bcdarwin
Copy link
Member

Having the build succeed implying that the tests passed is quite convenient, but perhaps I'm also missing the motivation.

One thing that would be useful is downloading a bunch of packages from Hydra but re-running the tests locally (but without recompiling) for packages that use CUDA or CPU-specific features.

Perhaps only vaguely related, but one thing I've wished for is an impureCheckPhase (surely proposed elsewhere) which would allow downloading data. A separate derivation would be one way (perhaps overkill) to prevent leaking nondeterminism from these impure tests into the package itself.

@stale
Copy link

stale bot commented Jun 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants