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

Track test derivations and parallelize building and testing #7662

Open
roberth opened this issue Jan 23, 2023 · 7 comments
Open

Track test derivations and parallelize building and testing #7662

roberth opened this issue Jan 23, 2023 · 7 comments
Labels
feature Feature request or proposal language The Nix expression language; parser, interpreter, primops, evaluation, etc store Issues and pull requests concerning the Nix store

Comments

@roberth
Copy link
Member

roberth commented Jan 23, 2023

Is your feature request related to a problem? Please describe.

Tests can be run as part of a package's main derivation (that produces the binaries, etc.), or the tests can be run separately.
Currently this presents a trade-off between build latency and reliability.

  • By putting the tests in a separate derivation, dependencies do not need to wait for the tests to run, allowing for increased parallelism, and a shorter critical path (ie shortening the chain of dependencies with the longest cumulative build time).
  • However, by putting them in a separate derivation, it's not guaranteed that all variations that package consumers come up with pass the tests. Such variations appear through flake follows, overlays, overrides, etc; so CI can not guarantee all to pass.

Describe the solution you'd like

Break the trade-off by tracking test dependencies in the string context.

  • string context: add a new "test item" constructor to the variant
  • builtins.derivationStrict: perform the build by ignoring the test items in the string context, but re-add these test items to the context of the returned output strings.
  • nix build, etc: do build the whole string context, including the test items
  • optionally, prioritize tests when scheduling the build graph, to fail early

This has the effect of making all the test derivations fairly independently schedulable tasks without the risk of skipping any.

Describe alternatives considered

Track it in a package ("passthru") attribute instead. This isn't airtight, because dependencies added through string interpolations won't be captured. Example:

''
  ${hello}/bin/hello >$out
''

In the above string, the test dependencies of hello aren't necessary for the string with context to be realized. Without adding them as regular dependencies (and therefore linearizing the build, which we don't want) it's not possible to extract test dependencies from the string. It's not feasible to do this manually with a helper function involving builtins.getContext and unsafeDiscardStringContext, because this helper would have to be used in far too many places. Furthermore, it has two return values, necessitating a let binding at every call site, making the thing far too cumbersome as well.

Additional context
Add any other context or screenshots about the feature request here.

Priorities

Add 👍 to issues you find important.

@roberth roberth added the feature Feature request or proposal label Jan 23, 2023
@roberth roberth added the language The Nix expression language; parser, interpreter, primops, evaluation, etc label Jan 18, 2024
@ghost
Copy link

ghost commented Jan 21, 2024

Both the upside and downside that you hypothesize have been confirmed "in the wild".

When the gcc bootstrap was split up into two separate derivations, the checkPhase had to be moved into yet a third derivation.

  • Upside: stdenv bootstraps almost 20% faster now, because the gcc tests (which do a complete rebuild of the compiler and then throw it away) run in parallel with everything that depends on gcc
  • Downside: "it's not guaranteed that all variations that package consumers come up with pass the tests." We had to manually add gcc-stageCompare to the Hydra release jobset. This means that basically only hydra.nixos.org ever runs these checks; hardly anybody else knows that they need to explicitly request them. I'm not happy about this.

I'm 👍 on making these kinds of "separate derivation" tests more automatic, but 👎 on string contexts in general. They are an icky hack that we recently discovered isn't even necessary (tvix and parnix don't have them) and can be omitted. I think this would be a bad time to go looking around for reasons to make them unremovable.

I don't really think there needs to be a nixlang/libexpr-level solution. The goal is to express the following:

Anytime you realize the build-derivation, you must also realize the test-derivation concurrently with any referrers of the build-derivation.

The word "concurrently" is what distinguishes this from a simple wrapper, which would cause the test-derivation to be realized in series with the build-derivations' referrers. So this is really a build-scheduling problem, and it seems like it calls for adding a new store derivation field, rather than a new behavior for derivation expressions.

Here's the litmus test for me: I could easily imagine utilizing this feature from some non-libexpr-using, raw-derivation-writing system (e.g. Guix or this example in RFC92). When that's true, I think it's a sign that the feature in question is a libstore thing rather than a libexpr thing.

@roberth
Copy link
Member Author

roberth commented Jan 21, 2024

String context in general

tl;dr agreed, may not be necessary!

but 👎 on string contexts in general. They are an icky hack

I haven't considered it to be an icky hack, as it removes a reliance on a form of global state, but that's interesting, to try and avoid it.
I've previously gathered input for extra use cases that could be built on top of string context - mostly:

However, alternative implementations also seem feasible, as follows

  • secrets: just 1 bit of information, so it could be represented as an extra Value constructer (e.g. tString and tSecret - both instances of builtins.typeOf x == "string")
  • license info: global mapping of drv path to meta attrs
  • impurity tainting: just like tSecret, but not a strong use case anyway...
  • exception message: just do rethrow primop #8689 instead - less "first class value" but still can't leak into happy path

Async test derivation relation in derivation

It would be unnecessary to invalidate output hashes based on such a new store derivation field.
This is another reason to improve the output hashing algorithm along with

An open question is who's responsible for running which tests. When tests are dependencies or phases, the existence of an output implies that the tests have completed successfully. Hence they are abstracted away.
In the new scheduling model with async test dependencies, this is not the case anymore. Running all tests in the entire derivation closure does not seem feasible.
Open questions:

  • When are the test derivations built/which derivations?
  • Alternatively, how do we cache tests that have succeeded? (Cache as in binary cache but verb)
  • Can we avoid having to evaluate the expressions for test derivations?
    Perhaps we could require that the new derivation field is a map, and then provide a mode where builtins.derivation only evaluates the test attributes whose names aren't in the cache yet. (For some suitable cache.) This mode won't be safe and disabling it entirely may become prohibitively slow. Maybe it needs to be manual invalidation where the user tells Nix the name of the derivation whose tests need to be reevaluated.

@roberth roberth changed the title Use string context to track test derivations and parallelize building and testing Track test derivations and parallelize building and testing Jan 21, 2024
@roberth roberth added the store Issues and pull requests concerning the Nix store label Jan 21, 2024
@Ericson2314
Copy link
Member

Ericson2314 commented Jan 21, 2024

Here is another one:

I am also skeptical about not having a string context keeping the same semantics. I bet I can find some bugs around spoofing store paths. Similar to discussion in-band vs out-of-band capabilities (not sure what terms are used for that).

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/why-does-the-nixos-infrastructure-have-to-be-hosted-in-a-centralized-way/46789/41

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/discourse-via-email/29/13

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/review-of-333575/50496/1

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/review-of-333575/50496/3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal language The Nix expression language; parser, interpreter, primops, evaluation, etc store Issues and pull requests concerning the Nix store
Projects
None yet
Development

No branches or pull requests

3 participants