-
-
Notifications
You must be signed in to change notification settings - Fork 324
Use nix-eval-jobs
and delete hydra-eval-jobs
#1421
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
A relevant thing to consider here is whether we want an official NixOS project to be dependant on a Nix Community project. As of now (afaik), the NixOS GitHub namespace is mostly self-contained. I see these solutions:
I personally prefer the latter two options over the first one since it would get rid of code duplication and remove a component from Hydra that only a few people are proficient in and that gets a lot less maintenance than nix-eval-jobs. Maybe that's a question for the new SC? |
Happy to move the project to the NixOS org if I can keep maintaining it there. We would loose aarch64-linux builds because those runners are currently sponsored by namespace in nix-community. I can attach my buildbot-nix installation to this org to compensate. |
That's more a technical question for the Hydra/Nix team, unless we feel to overwhelmed what to do and need to escalate. |
498b283
to
d0cfbed
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
d0cfbed
to
8a49ecb
Compare
OK undrafting because we're back on @dasJ I am fine resolving the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The perl bits look generally ok to me but not my expertise. Maybe put somewhere a bit clearer that the constituents are not supported anymore. That's not used on hydra.nixos.org, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated before, I do find this move to be a good idea. Here are some things I noticed while reading the code
return; | ||
} | ||
|
||
die "Jobset contains a job with an empty name. Make sure the jobset evaluates to an attrset of jobs.\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a relevant check? Kind of surprising to see it gone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ma27 Do you know about this? Maybe this is something that should be restored in the new streaming version? Or maybe its fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had to guess, this probably just got removed since $jobs
got replaced by an iterator?
But yeah, seems reasonalbe to add it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think it is gone because now we are steaming records, rather than sending a map. There is more outer key to check like this.
inputs.nix.inputs.libgit2.follows = "libgit2"; | ||
|
||
inputs.nix-eval-jobs.url = "github:nix-community/nix-eval-jobs"; | ||
inputs.nix-eval-jobs.inputs.nixpkgs.follows = "nixpkgs"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I forgot in the last review: I think we should also force the nix version of hydra into the package, otherwise we might depend on 2 nix versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually a good one and could be a problem if hydra or nix-eval-jobs doesn't keep up with nix releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can maintain release branches for different nix versions again if this is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the literal Nix derivation is forced to be the same, but the version is the same now.
This comment was marked as resolved.
This comment was marked as resolved.
a3e3fe9
to
d2227af
Compare
d2227af
to
d97956d
Compare
d97956d
to
cc5f805
Compare
OK thanks to the fix in |
cc5f805
to
890e693
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-12-09-nix-team-meeting-minutes-201/57280/1 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-12-13-nix-team-meeting-minutes-202/57281/1 |
Depends on nix-community/nix-eval-jobs#349 & NixOS#1421. Almost equivalent to NixOS#1425, but with a small change: when having e.g. an aggregate job with a glob that matches nothing, the jobset evaluation is failed now. This was the intended behavior before (hydra-eval-jobset fails hard if an aggregate is broken), the code-path was never reached however since the aggregate was never marked as broken in this case before.
5833e12
to
40fbc55
Compare
OK I tested and fixed Flake support, so I think this is ready! |
40fbc55
to
4c9d16e
Compare
(cherry picked from commit 684cc50)
incrementally ingest eval results nix-eval-jobs streams output, unlike hydra-eval-jobs. Now that we've migrated, we can use this to: 1. Use less RAM by avoiding buffering a whole eval's worth of metadata into a Perl string and an array of JSON objects. 2. Make evals latency a bit lower by allowing the queue runner to start ingesting builds faster. Also use the newly-restored constituents support in `nix-eval-jobs` Note, we pass --workers and --max-memory-size to n-e-j Lost in the h-e-j -> n-e-j migration, causing evaluation to always be single threaded and limited to 4GiB RAM. Follow the config settings like h-e-j used to do (via C++ code). `nix-eval-jobs` should check `hydraJobs` and then `checks` with flakes (cherry picked from commit 6d4ccff) (cherry picked from commit b0e9b4b) (cherry picked from commit cdfc5c81e8037d3e4818a3e459d0804b2c157ea9) (cherry picked from commit 4b107e6) Co-Authored-By: Maximilian Bosch <[email protected]>
(cherry picked from commit ed7c587)
4c9d16e
to
85383b9
Compare
Depends on nix-community/nix-eval-jobs#349 & NixOS#1421. Almost equivalent to NixOS#1425, but with a small change: when having e.g. an aggregate job with a glob that matches nothing, the jobset evaluation is failed now. This was the intended behavior before (hydra-eval-jobset fails hard if an aggregate is broken), the code-path was never reached however since the aggregate was never marked as broken in this case before.
Hydra unilaterally removed hydra-eval-jobs, so we need to figure out how to migrate to nix-eval-jobs. Performance seems to be worse for our use case somehow. NixOS/hydra#1421
Hydra unilaterally removed hydra-eval-jobs, so we need to figure out how to migrate to nix-eval-jobs. I can't shake the feeling that it's slower. Maybe we need to increase the resource limitations for nix-eval-jobs. nix-eval-jobs no longer produces a big JSON object, but instead one object per line (one for each job). This is supported in a simple way by readJSONLinesProcess. It'd be possible to implement this without presupposing that there's one object per line, however, it is not an usecase exactly intended by aeson, it seems. nix-eval-jobs makes our job easier in some ways, e.g. jobs have a proper meta set now, so we no longer need to cross reference a mail address to github handle map. There is even room for further improvement, e.g. attribute paths can just be queried instead of generating them using Text.splitOn. See also NixOS/hydra#1421.
Hydra unilaterally removed hydra-eval-jobs, so we need to figure out how to migrate to nix-eval-jobs. I can't shake the feeling that it's slower. Maybe we need to increase the resource limitations for nix-eval-jobs. nix-eval-jobs no longer produces a big JSON object, but instead one object per line (one for each job). This is supported in a simple way by readJSONLinesProcess. It'd be possible to implement this without presupposing that there's one object per line, however, it is not an usecase exactly intended by aeson, it seems. nix-eval-jobs makes our job easier in some ways, e.g. jobs have a proper meta set now, so we no longer need to cross reference a mail address to github handle map. There is even room for further improvement, e.g. attribute paths can just be queried instead of generating them using Text.splitOn. See also NixOS/hydra#1421.
Hydra unilaterally removed hydra-eval-jobs, so we need to figure out how to migrate to nix-eval-jobs. I can't shake the feeling that it's slower. Maybe we need to increase the resource limitations for nix-eval-jobs. nix-eval-jobs no longer produces a big JSON object, but instead one object per line (one for each job). This is supported in a simple way by readJSONLinesProcess. It'd be possible to implement this without presupposing that there's one object per line, however, it is not an usecase exactly intended by aeson, it seems. nix-eval-jobs makes our job easier in some ways, e.g. jobs have a proper meta set now, so we no longer need to cross reference a mail address to github handle map. There is even room for further improvement, e.g. attribute paths can just be queried instead of generating them using Text.splitOn. See also NixOS/hydra#1421.
nix-eval-jobs
was forked fromhydra-eval-jobs
originally, for others to use. That's great! But maintaining two copies is no good. But we use that.Original author is @delroth, for Lix's Hydra, thank you!
Draft because some tests fail. I am not sure why. Maybe there are fixes on the Lix branch that can be cherry-picked to fix? Not sure!