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

Parallel GH actions workflow for Nixpkgs eval #356023

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Nov 14, 2024

This PR primarily introduces a new GitHub Action workflow to effectively do a full Hydra evaluation for every PR, paving the way towards relieving ofborg of its evaluation duty. This is motivated by https://discourse.nixos.org/t/infrastructure-announcement-the-future-of-ofborg-your-help-needed/56025, see also #355847.

To make this as efficient as possible, 4 workflow jobs are spawned in parallel (one for each supported system, as originally experimented by @Mic92 in #352808), while each job makes full use of its 4 cores and 16GB of memory (based off amjoseph's idea in #269403 and initial work in #269356).

I've also implemented it in a way that limits maximum memory usage as Nixpkgs grows. CI will only take longer to finish, instead of OOMing. Currently it takes about 5 minutes, check out this test run.

Note that ofborg does more than just evaluate Nixpkgs, but this PR can be used as a base to also implement output path comparisons to assign labels based on changed paths, to request appropriate maintainers, and do evaluation time comparisons.

Ping @philiptaron @Mic92 @Erethon @balsoft @JohnRTitor @a-kenji

Things done


This work is funded by Tweag and Antithesis

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: policy discussion 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions labels Nov 14, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 15, 2024
.github/workflows/eval.yml Outdated Show resolved Hide resolved
.github/workflows/eval.yml Outdated Show resolved Hide resolved
@Mic92
Copy link
Member

Mic92 commented Nov 16, 2024

I added several improvements to this script: tweag#100 notably more useful json output.

@Mic92
Copy link
Member

Mic92 commented Nov 16, 2024

I like this approach already. I want to incorporate this code in one form or the other in nixpkgs-review as it will make evaluating on normal laptops possible again.

@JohnRTitor
Copy link
Contributor

Please do! Currently evaluating uses too much memory and time!

@Mic92
Copy link
Member

Mic92 commented Nov 16, 2024

@GaetanLepage wants to look into integrating it into nixpkgs-review.

@Mic92
Copy link
Member

Mic92 commented Nov 17, 2024

@infinisil I would suggest to just address all comments for this pull request for now and maybe merge my pull request but not to extend the scope for the first pull request too much.
It would be good to first test to see how this action scales to the whole org before we proceed with more features.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/infrastructure-announcement-the-future-of-ofborg-your-help-needed/56025/37

ci/parallel.nix Outdated Show resolved Hide resolved
@JohnRTitor
Copy link
Contributor

JohnRTitor commented Nov 17, 2024

It would be good to first test to see how this action scales to the whole org before we proceed with more features.

Agreed, due to slowness of Ofborg recently a few commiters are merging PRs without waiting for a successful eval which can take up to 13-15 hours. (I am guilty of the act myself)

This fast but dangerous approach of review makes nixpkgs prone to a tree with broken eval.

Having this basic CI check would help to detect issues beforehand, as we plan to phase out Ofborg in less than two months.

@infinisil infinisil force-pushed the gha-eval branch 4 times, most recently from 50e150a to 4622f17 Compare November 18, 2024 08:42
@infinisil
Copy link
Member Author

infinisil commented Nov 18, 2024

I did a rewrite and a rebase, check out https://github.com/tweag/nixpkgs/actions/runs/11888336167?pr=99 now, you can download the computed outpaths!

Still left to do is some docs and some final cleanup, but otherwise I'm fairly content with this.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 18, 2024
@Mic92
Copy link
Member

Mic92 commented Nov 19, 2024

The nixpkgs-review counterpart is being worked on here: Mic92/nixpkgs-review#429
This will bring both worlds closer together.

…ttrNamesOnly

The attrNamesOnly feature is used by
pkgs/top-level/release-attrpaths-superset.nix to return a superset of
all attributes that might be built by Hydra.

Before this change it would include all attribute paths to derivations that could
be found recursively, ignoring the recurseForDerivations and
recurseForRelease attributes that control Hydra's recursion.

This had the effect that it would end up with ~266000 attributes, most of which definitely won't be built by Hydra. We can remove those while staying true to the superset notion to end up with just ~97000, a reduction of ~63.6%! This also comes with an eval time reduction from 31.7s to 18.7s (-41.0%)!

As an example, all derivations in pypy310Packages don't get included
anymore, because it doesn't have a `.recurseForDerivations` set.

As a nice side effect, with `--arg enableWarnings false`, no more
warnings are printed, because things like
`checkpointBuildTools.mkCheckpointedBuild` (which is deprecated) isn't
being recursed to anymore.
@infinisil infinisil changed the title [WIP] Parallel GH actions workflow for Nixpkgs eval Parallel GH actions workflow for Nixpkgs eval Nov 20, 2024
@infinisil infinisil force-pushed the gha-eval branch 3 times, most recently from 7c36b3d to 9360b35 Compare November 20, 2024 06:20
@infinisil infinisil marked this pull request as ready for review November 20, 2024 06:20
@infinisil
Copy link
Member Author

infinisil commented Nov 20, 2024

This is done now imo! I added some docs and some comments, I also updated the PR description. Pinging @Mic92 @Erethon @balsoft @JohnRTitor @a-kenji @azuwis @TheGiraffe3 for review and approval

@@ -0,0 +1,19 @@
# Nixpkgs CI evaluation

The code in this directory is used by the [eval.yml](../../.github/workflows/eval.yml) GitHub Actions workflow to evaluate the majority of Nixpkgs for all PRs, effectively making sure that when the development branches are processed by Hydra, it doesn't encounter any evaluation failures.

Choose a reason for hiding this comment

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

Suggested change
The code in this directory is used by the [eval.yml](../../.github/workflows/eval.yml) GitHub Actions workflow to evaluate the majority of Nixpkgs for all PRs, effectively making sure that when the development branches are processed by Hydra, it doesn't encounter any evaluation failures.
The code in this directory is used by the [eval.yml](../../.github/workflows/eval.yml) GitHub Actions workflow to evaluate the majority of Nixpkgs for all PRs, effectively making sure that when the development branches are processed by Hydra, they don't encounter any evaluation failures.

Is "branches" or "Hydra" the subject?

Copy link
Member Author

Choose a reason for hiding this comment

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

Meant for it to be Hydra, how about avoiding the ambiguity by getting rid of the thing that could be ambiguous haha:

Suggested change
The code in this directory is used by the [eval.yml](../../.github/workflows/eval.yml) GitHub Actions workflow to evaluate the majority of Nixpkgs for all PRs, effectively making sure that when the development branches are processed by Hydra, it doesn't encounter any evaluation failures.
The code in this directory is used by the [eval.yml](../../.github/workflows/eval.yml) GitHub Actions workflow to evaluate the majority of Nixpkgs for all PRs, effectively making sure that when the development branches are processed by Hydra, no evaluation failures are encountered.

Choose a reason for hiding this comment

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

Yep, your suggestion looks good to me.

ci/eval/README.md Outdated Show resolved Hide resolved
ci/supportedSystems.nix Outdated Show resolved Hide resolved
else if value.recurseForDerivations or false || value.recurseForRelease or false then
packagePlatforms value
recursiveMapPackages f value

Choose a reason for hiding this comment

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

Apologies if I'm missing something, but does the "f" do anything?

Suggested change
recursiveMapPackages f value
recursiveMapPackages value

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bit confusing, but recursiveMapPackages takes two arguments, because it's defined like recursiveMapPackages f = mapAttrs (...), which is the same as recursiveMapPackages f attrs = mapAttrs (...) attrs. So here it's called recursively with the same f but a different attrs.

Motivated by ofborg struggling [1] and its evaluations taking too long,
inspired by Jörg's initial PR [2]
and Adam's previous attempt to parallelise Nixpkgs evaluation [3],
this PR contains initial work to relief ofborg from its evaluation duty
by using GitHub Actions to evaluate Nixpkgs.

For now this doesn't take care of all of what ofborg does, such as
requesting appropriate reviewers or labeling mass rebuilds, but this can
be follow-up work.

[1]: https://discourse.nixos.org/t/infrastructure-announcement-the-future-of-ofborg-your-help-needed/56025?u=infinisil
[2]: NixOS#352808
[3]: NixOS#269403

Co-Authored-By: Jörg Thalheim <joerg@thalheim.io>
Co-Authored-By: Adam Joseph <adam@westernsemico.com>
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.

6 participants