ci/eval: re-implement compare in nix by GaetanLepage · Pull Request #362844 · NixOS/nixpkgs

GaetanLepage · 2024-12-07T15:35:40Z

Things done

This is a reimplementation in nix of the current compare.jq logic.
Later, I will add more information in the produced JSON (so that we can use this in nixpkgs-review).
For now, this is a strict equivalent of the previous jq implementation.

There is one logic change that might be acceptable:
If a package foo existed, but was not available on platform aarch64-darwin and the PR makes it available for this platform, should it be listed as an added package or as a changed one ?
The current implementation lists it in changed while the new one consider puts it in changed.
This should not be too hard to change.

Please, feel free to criticize the implementation details and propose some refactoring.
It might lack some commenting and also variable names could be wiser.

cc @Mic92 @infinisil

Add a 👍 reaction to pull requests you find important.

GaetanLepage · 2024-12-07T15:44:48Z

ci/eval/compare/default.nix

We could put those in the toJSON call below but it would require a rec. I don't know what is the recommended way.

JohnRTitor

I would open a test PR targeting your fork's branch, to test the action, if I were you.

infinisil

As long as it works, both in GHA's and using a local nix-build ci -A eval.compare ..., this LGTM!

paparodeo · 2024-12-07T16:22:31Z

Looks good. FWIW I did this in sql and added stats ex1, ex2, ex3, ex4.

I think the rebuilds should be done by paths rather than attributes given there is a many to one mapping of attrs to paths. ofborg will treat a new for platform path as a new package (it gets the new package label).

JohnRTitor · 2024-12-07T17:18:09Z

added stats ex1, ex2, ex3, ex4.

looks superb to me!

GaetanLepage · 2024-12-07T21:27:01Z

Looks good. FWIW I did this in sql and added stats ex1, ex2, ex3, ex4.

Oh wow, this looks fancy !
Would you prefer that we have your implementation instead of mine ?
The motivation behind this PR is to later be able to add more information in the json (i.e. what to rebuild for each platform).

I think the rebuilds should be done by paths rather than attributes given there is a many to one mapping of attrs to paths. ofborg will treat a new for platform path as a new package (it gets the new package label).

It is kind of already the case no ? Here are the definitions I went with. Note: By "item" I mean a tuple (platform, name) (ex: python312Packages.torch.x86_64-linux).

added: any item that was not present before and now exists.
removed: any item that was present before and is not anymore.
changed: any item that was present before and is still there but which value has changed. By value, I mean its set of out-paths.

At this stage, tinymist.x86_64-linux could be changed, while tinymist.x86_64-darwin removed and tinymist.aarch64-darwin added.

Now, where this gets slightly tricky is that we don't report those affected items directly but only their names. I.e. we need to get rid of the platform. While in most cases, a single name will only end up in a single category, our (artificial) example of tinymist is hard to categorize.
Currently, the implementation will extract, for each category (added, removed and changed) the set of package names.
This means, that tinymist would here be listed in all three categories.

Having said that, this is not too crucial because what really counts is the number of rebuilds. The definition of rebuilds is precise and consensual:

#rebuilds = |added| + |changed|

where added and changed are items sets and are disjoint by construction.

GaetanLepage · 2024-12-07T22:24:55Z

@infinisil if you are happy with it, feel free to merge this PR.
I would directly open a follow up to add a raw-er list of "rebuilds" to the JSON so that we can leverage this result in nixpkgs-review.

paparodeo · 2024-12-07T22:57:06Z

Looks good. FWIW I did this in sql and added stats ex1, ex2, ex3, ex4.

Oh wow, this looks fancy ! Would you prefer that we have your implementation instead of mine ? The motivation behind this PR is to later be able to add more information in the json (i.e. what to rebuild for each platform).

It is up to the CI team tho I get the impression that a nix implementation is preferred. I find duckdb is pretty handy to query json files but am not sure how others feel about sql.

the sql implementation emits a json file which can be use to query what to build for each platform. eg:

$ jq -r '.[]|select(.action != "remove")| [.machine + "-" + .os, .attr]|@tsv' < changed.json |head
aarch64-darwin  _64gram
x86_64-darwin   _64gram
aarch64-darwin  bolt_19
aarch64-linux   bolt_19
x86_64-darwin   bolt_19
x86_64-linux    bolt_19
aarch64-darwin  bpf-linker
aarch64-linux   bpf-linker
x86_64-darwin   bpf-linker
x86_64-linux    bpf-linker

I think the rebuilds should be done by paths rather than attributes given there is a many to one mapping of attrs to paths. ofborg will treat a new for platform path as a new package (it gets the new package label).

It is kind of already the case no ? Here are the definitions I went with. Note: By "item" I mean a tuple (platform, name) (ex: python312Packages.torch.x86_64-linux).

added: any item that was not present before and now exists.

removed: any item that was present before and is not anymore.

changed: any item that was present before and is still there but which value has changed. By value, I mean its set of out-paths.

At this stage, tinymist.x86_64-linux could be changed, while tinymist.x86_64-darwin removed and tinymist.aarch64-darwin added.

Now, where this gets slightly tricky is that we don't report those affected items directly but only their names. I.e. we need to get rid of the platform. While in most cases, a single name will only end up in a single category, our (artificial) example of tinymist is hard to categorize. Currently, the implementation will extract, for each category (added, removed and changed) the set of package names. This means, that tinymist would here be listed in all three categories.

Having said that, this is not too crucial because what really counts is the number of rebuilds. The definition of rebuilds is precise and consensual:
#rebuilds = |added| + |removed|
where added and removed are items sets and are disjoint by construction.

did you mean #rebuilds = |added| + |changed|? this double counts as there are some attributes which map to the same path. eg: llvm_19 llvmPackages_19.llvm and llvmPackages_19.libllvm are all the same package and should only be counted as 1 rebuild but I believe that this PR will count it as 3.

can see the many to 1 mapping when looking at the output of a llvm-19 upgrade https://github.com/paparodeo/nixpkgs/actions/runs/12214197953

$ unzip comparison.zip 
$ duckdb
v1.1.3 19864453f7
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
D SELECT
    attr,
    count(*) over ( PARTITION BY paths ) AS c,
    format('{}-{}', machine, os) AS target,
    md5(paths::string) AS md5,
  FROM 'changed.json'
  QUALIFY c > 2
  ORDER BY target, paths, attr;
┌────────────────────────────────┬───────┬────────────────┬──────────────────────────────────┐
│              attr              │   c   │     target     │               md5                │
│            varchar             │ int64 │    varchar     │             varchar              │
├────────────────────────────────┼───────┼────────────────┼──────────────────────────────────┤
│ llvmPackages_19.libllvm        │     3 │ aarch64-darwin │ cb679493005b2b6a9b0aa761526f2c76 │
│ llvmPackages_19.llvm           │     3 │ aarch64-darwin │ cb679493005b2b6a9b0aa761526f2c76 │
│ llvm_19                        │     3 │ aarch64-darwin │ cb679493005b2b6a9b0aa761526f2c76 │
│ clang_19                       │     3 │ aarch64-darwin │ 2a8c35d601d4e04e5554cf67c1c4ba64 │
│ llvmPackages_19.clang          │     3 │ aarch64-darwin │ 2a8c35d601d4e04e5554cf67c1c4ba64 │
│ llvmPackages_19.libcxxClang    │     3 │ aarch64-darwin │ 2a8c35d601d4e04e5554cf67c1c4ba64 │
│ clang_19                       │     3 │ aarch64-linux  │ 1862f8c3f5abde44aa4488da5dd9824e │
│ llvmPackages_19.clang          │     3 │ aarch64-linux  │ 1862f8c3f5abde44aa4488da5dd9824e │
│ llvmPackages_19.libstdcxxClang │     3 │ aarch64-linux  │ 1862f8c3f5abde44aa4488da5dd9824e │
│ llvmPackages_19.libllvm        │     3 │ aarch64-linux  │ bc1f129cf02468ed504b13a6a5443259 │
│ llvmPackages_19.llvm           │     3 │ aarch64-linux  │ bc1f129cf02468ed504b13a6a5443259 │
│ llvm_19                        │     3 │ aarch64-linux  │ bc1f129cf02468ed504b13a6a5443259 │
│ clang_19                       │     3 │ x86_64-darwin  │ 48872f71dbcd65d57932fba81972b068 │
│ llvmPackages_19.clang          │     3 │ x86_64-darwin  │ 48872f71dbcd65d57932fba81972b068 │
│ llvmPackages_19.libcxxClang    │     3 │ x86_64-darwin  │ 48872f71dbcd65d57932fba81972b068 │
│ llvmPackages_19.libllvm        │     3 │ x86_64-darwin  │ b18287690eda354205d320f1429a1f44 │
│ llvmPackages_19.llvm           │     3 │ x86_64-darwin  │ b18287690eda354205d320f1429a1f44 │
│ llvm_19                        │     3 │ x86_64-darwin  │ b18287690eda354205d320f1429a1f44 │
│ llvmPackages_19.libllvm        │     3 │ x86_64-linux   │ 40a63b8cf2fac5fbe4a4f3623bb10b52 │
│ llvmPackages_19.llvm           │     3 │ x86_64-linux   │ 40a63b8cf2fac5fbe4a4f3623bb10b52 │
│ llvm_19                        │     3 │ x86_64-linux   │ 40a63b8cf2fac5fbe4a4f3623bb10b52 │
│ clang_19                       │     3 │ x86_64-linux   │ d1a744b78d295a6675f8178ab9b9dcc5 │
│ llvmPackages_19.clang          │     3 │ x86_64-linux   │ d1a744b78d295a6675f8178ab9b9dcc5 │
│ llvmPackages_19.libstdcxxClang │     3 │ x86_64-linux   │ d1a744b78d295a6675f8178ab9b9dcc5 │
├────────────────────────────────┴───────┴────────────────┴──────────────────────────────────┤
│ 24 rows                                                                          4 columns │
└────────────────────────────────────────────────────────────────────────────────────────────┘

GaetanLepage · 2024-12-07T23:02:06Z

It is up to the CI team tho I get the impression that a nix implementation is preferred.

Definitely.

I find duckdb is pretty handy to query json files but am not sure how others feel about sql.

Your implementation looks very nice indeed ! I am not too familiar with SQL and would be unable to maintain it. However if it does the job and is preferred by the CI team, I would be fine with it of course.

did you mean #rebuilds = |added| + |changed|?

Yes. I have corrected my previous message, thanks.

this double counts as there are some attributes which map to the same path. eg: llvm_19 llvmPackages_19.llvm and llvmPackages_19.libllvm are all the same package and should only be counted as 1 rebuild but I believe that this PR will count it as 3.

Very true. Now I understand your many-to-one point. I think that the current implem also suffers from this issue. I don't know how crucial it is though. Of course, having an accurate solution would be better though.

Mic92 · 2024-12-08T00:25:06Z

Nix is probably a better cultural fit looking for the contributors than sql.

paparodeo · 2024-12-08T01:17:37Z

this double counts as there are some attributes which map to the same path. eg: llvm_19 llvmPackages_19.llvm and llvmPackages_19.libllvm are all the same package and should only be counted as 1 rebuild but I believe that this PR will count it as 3.

Very true. Now I understand your many-to-one point. I think that the current implem also suffers from this issue. I don't know how crucial it is though. Of course, having an accurate solution would be better though.

yes the current implementation also has this problem. also seemed like it missed a rebuild when the meta.platforms changed from linux -> unix. eg: https://github.com/NixOS/nixpkgs/actions/runs/12178480057 the labels are now correct only due to ofborg but if looking at the changed.json it lists:

  "labels": [
    "10.rebuild-linux: 1-10",
    "10.rebuild-darwin: 0"
  ]

but darwin should also get a rebuild -- not sure if this PR has the same issue. the sql does the right thing given the same inputs.

paparodeo · 2024-12-08T02:03:47Z

not sure if this PR has the same issue

this PR seems to handle https://github.com/NixOS/nixpkgs/actions/runs/12178480057 correctly (master marks darwin as 0 builds). so that's good!.

{
  "attrdiff": {
    "added": [
      "netris"
    ],
    "changed": [
      "netris"
    ],
    "removed": []
  },
  "labels": {
    "darwin": "10.rebuild-darwin: 1-10",
    "linux": "10.rebuild-linux: 1-10"
  },
  "rebuildCountByKernel": {
    "darwin": 1,
    "linux": 1
  },
  "rebuildsByKernel": {
    "darwin": [
      "netris"
    ],
    "linux": [
      "netris"
    ]
  }
}

ci/eval/compare/utils.nix

ci/eval/compare/default.nix

GaetanLepage · 2024-12-09T19:49:54Z

Any further feedback/requests on this ?

Mic92 · 2024-12-09T19:55:17Z

Doing a test run in Mic92#40.
Than we can merge it.

paparodeo · 2024-12-10T08:33:53Z

seems to be mis-labeling: https://github.com/NixOS/nixpkgs/actions/runs/12252092760

listed as 101-500 but should be 11-100

"labels": {
    "darwin": "10.rebuild-darwin: 101-500",
    "linux": "10.rebuild-linux: 101-500"
  },
  "rebuildCountByKernel": {
    "darwin": 11,
    "linux": 12
  },

diff --git a/ci/eval/compare/utils.nix b/ci/eval/compare/utils.nix
index 22353499d699..2794b5eda4ec 100644
--- a/ci/eval/compare/utils.nix
+++ b/ci/eval/compare/utils.nix
@@ -113,6 +113,8 @@ rec {
           "0"
         else if rebuildCount <= 10 then
           "1-10"
+        else if rebuildCount <= 100 then
+          "11-100"
         else if rebuildCount <= 500 then
           "101-500"
         else if rebuildCount <= 1000 then

GaetanLepage · 2024-12-10T08:54:26Z

seems to be mis-labeling: https://github.com/NixOS/nixpkgs/actions/runs/12252092760

listed as 101-500 but should be 11-100
"labels": {
    "darwin": "10.rebuild-darwin: 101-500",
    "linux": "10.rebuild-linux: 101-500"
  },
  "rebuildCountByKernel": {
    "darwin": 11,
    "linux": 12
  },

Indeed, the rebuildCountByKernel is fine, so it's a bug in the label assigning code...

GaetanLepage requested review from JohnRTitor, Mic92 and infinisil December 7, 2024 15:35

github-actions bot added the 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions label Dec 7, 2024

GaetanLepage force-pushed the compare-nix branch from a52d86c to 99b6127 Compare December 7, 2024 15:36

nix-owners bot requested review from LeSuisse, mweinelt, philiptaron and risicle December 7, 2024 15:37

github-actions 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 Dec 7, 2024

GaetanLepage force-pushed the compare-nix branch from 99b6127 to 0474aa6 Compare December 7, 2024 15:43

GaetanLepage commented Dec 7, 2024

View reviewed changes

JohnRTitor reviewed Dec 7, 2024

View reviewed changes

GaetanLepage force-pushed the compare-nix branch from 0474aa6 to 4abc536 Compare December 7, 2024 15:48

infinisil approved these changes Dec 7, 2024

View reviewed changes

paparodeo approved these changes Dec 8, 2024

View reviewed changes

Mic92 reviewed Dec 8, 2024

View reviewed changes

ci/eval/compare/utils.nix Outdated Show resolved Hide resolved

Mic92 reviewed Dec 8, 2024

View reviewed changes

ci/eval/compare/default.nix Outdated Show resolved Hide resolved

ci/eval: re-implement compare in nix

f94b4bd

GaetanLepage force-pushed the compare-nix branch from 4abc536 to f94b4bd Compare December 8, 2024 09:23

GaetanLepage requested a review from Mic92 December 9, 2024 17:06

Mic92 merged commit 23c1faa into NixOS:master Dec 9, 2024

GaetanLepage deleted the compare-nix branch December 9, 2024 20:04

GaetanLepage mentioned this pull request Dec 9, 2024

ci/eval: add rebuildsByPlatform to the comparison result #363751

Merged

13 tasks

This was referenced Dec 10, 2024

ci/eval: fix compare label assignment #363851

Merged

python312Packages.langgraph*: 20241206 update #362648

Merged

infinisil mentioned this pull request Dec 11, 2024

ci/eval: Also count added packages as rebuilds #361206

Merged

1 task

getchoo mentioned this pull request Jan 1, 2025

[Backport release-24.11] GitHub Actions PRs #369914

Merged

13 tasks

JohnRTitor mentioned this pull request Jan 27, 2025

Replacing ofBorg with GitHub Actions #355847

Closed

7 tasks

Uh oh!

Conversation

GaetanLepage commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things done

Uh oh!

GaetanLepage Dec 7, 2024

Choose a reason for hiding this comment

Uh oh!

JohnRTitor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infinisil left a comment

Choose a reason for hiding this comment

Uh oh!

paparodeo commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohnRTitor commented Dec 7, 2024

Uh oh!

GaetanLepage commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GaetanLepage commented Dec 7, 2024

Uh oh!

paparodeo commented Dec 7, 2024

Uh oh!

GaetanLepage commented Dec 7, 2024

Uh oh!

Mic92 commented Dec 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paparodeo commented Dec 8, 2024

Uh oh!

paparodeo commented Dec 8, 2024

Uh oh!

Uh oh!

Uh oh!

GaetanLepage commented Dec 9, 2024

Uh oh!

Mic92 commented Dec 9, 2024

Uh oh!

paparodeo commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GaetanLepage commented Dec 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GaetanLepage commented Dec 7, 2024 •

edited

Loading

JohnRTitor left a comment •

edited

Loading

paparodeo commented Dec 7, 2024 •

edited

Loading

GaetanLepage commented Dec 7, 2024 •

edited

Loading

Mic92 commented Dec 8, 2024 •

edited

Loading

paparodeo commented Dec 10, 2024 •

edited

Loading