Skip to content

Commit 8a39ce4

Browse files
workflows/eval: diff outpaths immediately
This moves the diff of outpaths into the outpaths job, mainly as a preparation to allow future improvements. For example, this will allow running the purity release checks only on changed outpaths instead of the whole eval. This also removes the inefficiency introduced in the last commit about uploading the intermediate paths twice. Now, only the diff is passed on. Also, technically, the diff is now run in parallel across 4 jobs. This should be *slightly* faster than before, where outpaths from all systems were combined first and then diffed. It's probably only a few seconds, though.
1 parent a6b659b commit 8a39ce4

File tree

5 files changed

+117
-79
lines changed

5 files changed

+117
-79
lines changed

.github/workflows/eval.yml

Lines changed: 21 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -135,31 +135,35 @@ jobs:
135135
github-token: ${{ github.token }}
136136
merge-multiple: true
137137

138-
- name: Upload the output paths and eval stats
138+
- name: Compare outpaths against the target branch
139+
if: steps.targetRunId.outputs.targetRunId
140+
env:
141+
MATRIX_SYSTEM: ${{ matrix.system }}
142+
run: |
143+
nix-build untrusted/ci -A eval.diff \
144+
--arg beforeDir ./target \
145+
--arg afterDir "$(readlink ./merged)" \
146+
--argstr evalSystem "$MATRIX_SYSTEM" \
147+
--out-link diff
148+
149+
- name: Upload outpaths diff and stats
139150
if: steps.targetRunId.outputs.targetRunId
140151
uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
141152
with:
142-
name: target-${{ matrix.system }}
143-
path: target/*
153+
name: diff-${{ matrix.system }}
154+
path: diff/*
144155

145156
process:
146157
name: Process
147158
runs-on: ubuntu-24.04-arm
148159
needs: [ prepare, outpaths ]
149160
if: needs.prepare.outputs.targetSha
150161
steps:
151-
- name: Download output paths and eval stats for all systems (PR)
152-
uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4.1.8
153-
with:
154-
pattern: merged-*
155-
path: merged
156-
merge-multiple: true
157-
158-
- name: Download output paths and eval stats for all systems (target)
162+
- name: Download output paths and eval stats for all systems
159163
uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4.1.8
160164
with:
161-
pattern: target-*
162-
path: target
165+
pattern: diff-*
166+
path: diff
163167
merge-multiple: true
164168

165169
- name: Check out the PR at the target commit
@@ -173,18 +177,11 @@ jobs:
173177
with:
174178
extra_nix_config: sandbox = true
175179

176-
- name: Combine all output paths and eval stats (PR)
177-
run: |
178-
nix-build trusted/ci -A eval.combine \
179-
--arg evalDir ./merged \
180-
--out-link combinedMerged
181-
182-
- name: Combine all output paths and eval stats (target)
183-
if: needs.prepare.outputs.targetSha
180+
- name: Combine all output paths and eval stats
184181
run: |
185182
nix-build trusted/ci -A eval.combine \
186-
--arg evalDir ./target \
187-
-o combinedTarget
183+
--arg diffDir ./diff \
184+
--out-link combined
188185
189186
- name: Compare against the target branch
190187
env:
@@ -196,8 +193,7 @@ jobs:
196193
197194
# Use the target branch to get accurate maintainer info
198195
nix-build trusted/ci -A eval.compare \
199-
--arg beforeDir "$(realpath combinedTarget)" \
200-
--arg afterDir "$(realpath combinedMerged)" \
196+
--arg combinedDir "$(realpath ./combined)" \
201197
--arg touchedFilesJson ./touched-files.json \
202198
--argstr githubAuthorId "$AUTHOR_ID" \
203199
--out-link comparison

ci/eval/compare/default.nix

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,7 @@
77
python3,
88
}:
99
{
10-
beforeDir,
11-
afterDir,
10+
combinedDir,
1211
touchedFilesJson,
1312
githubAuthorId,
1413
byName ? false,
@@ -66,30 +65,17 @@ let
6665
Example: { name = "python312Packages.numpy"; platform = "x86_64-linux"; }
6766
*/
6867
inherit (import ./utils.nix { inherit lib; })
69-
diff
7068
groupByKernel
7169
convertToPackagePlatformAttrs
7270
groupByPlatform
7371
extractPackageNames
7472
getLabels
7573
;
7674

77-
getAttrs =
78-
dir:
79-
let
80-
raw = builtins.readFile "${dir}/outpaths.json";
81-
# The file contains Nix paths; we need to ignore them for evaluation purposes,
82-
# else there will be a "is not allowed to refer to a store path" error.
83-
data = builtins.unsafeDiscardStringContext raw;
84-
in
85-
builtins.fromJSON data;
86-
beforeAttrs = getAttrs beforeDir;
87-
afterAttrs = getAttrs afterDir;
88-
8975
# Attrs
9076
# - keys: "added", "changed" and "removed"
9177
# - values: lists of `packagePlatformPath`s
92-
diffAttrs = diff beforeAttrs afterAttrs;
78+
diffAttrs = builtins.fromJSON (builtins.readFile "${combinedDir}/combined-diff.json");
9379

9480
rebuilds = diffAttrs.added ++ diffAttrs.changed;
9581
rebuildsPackagePlatformAttrs = convertToPackagePlatformAttrs rebuilds;
@@ -149,8 +135,8 @@ runCommand "compare"
149135
maintainers = builtins.toJSON maintainers;
150136
passAsFile = [ "maintainers" ];
151137
env = {
152-
BEFORE_DIR = "${beforeDir}";
153-
AFTER_DIR = "${afterDir}";
138+
BEFORE_DIR = "${combinedDir}/before";
139+
AFTER_DIR = "${combinedDir}/after";
154140
};
155141
}
156142
''

ci/eval/compare/utils.nix

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -93,32 +93,6 @@ rec {
9393
in
9494
uniqueStrings (builtins.map (p: p.name) packagePlatformAttrs);
9595

96-
/*
97-
Computes the key difference between two attrs
98-
99-
{
100-
added: [ <keys only in the second object> ],
101-
removed: [ <keys only in the first object> ],
102-
changed: [ <keys with different values between the two objects> ],
103-
}
104-
*/
105-
diff =
106-
let
107-
filterKeys = cond: attrs: lib.attrNames (lib.filterAttrs cond attrs);
108-
in
109-
old: new: {
110-
added = filterKeys (n: _: !(old ? ${n})) new;
111-
removed = filterKeys (n: _: !(new ? ${n})) old;
112-
changed = filterKeys (
113-
n: v:
114-
# Filter out attributes that don't exist anymore
115-
(new ? ${n})
116-
117-
# Filter out attributes that are the same as the new value
118-
&& (v != (new.${n}))
119-
) old;
120-
};
121-
12296
/*
12397
Group a list of `packagePlatformAttr`s by platforms
12498

ci/eval/default.nix

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -191,9 +191,11 @@ let
191191
cat "$chunkOutputDir"/result/* | jq -s 'add | map_values(.outputs)' > $out/${evalSystem}/paths.json
192192
'';
193193

194+
diff = callPackage ./diff.nix { };
195+
194196
combine =
195197
{
196-
evalDir,
198+
diffDir,
197199
}:
198200
runCommand "combined-eval"
199201
{
@@ -205,12 +207,22 @@ let
205207
mkdir -p $out
206208
207209
# Combine output paths from all systems
208-
cat ${evalDir}/*/paths.json | jq -s add > $out/outpaths.json
210+
cat ${diffDir}/*/diff.json | jq -s '
211+
reduce .[] as $item ({}; {
212+
added: (.added + $item.added),
213+
changed: (.changed + $item.changed),
214+
removed: (.removed + $item.removed)
215+
})
216+
' > $out/combined-diff.json
209217
210-
mkdir -p $out/stats
218+
mkdir -p $out/before/stats
219+
for d in ${diffDir}/before/*; do
220+
cp -r "$d"/stats-by-chunk $out/before/stats/$(basename "$d")
221+
done
211222
212-
for d in ${evalDir}/*; do
213-
cp -r "$d"/stats-by-chunk $out/stats/$(basename "$d")
223+
mkdir -p $out/after/stats
224+
for d in ${diffDir}/after/*; do
225+
cp -r "$d"/stats-by-chunk $out/after/stats/$(basename "$d")
214226
done
215227
'';
216228

@@ -225,25 +237,34 @@ let
225237
quickTest ? false,
226238
}:
227239
let
228-
evals = symlinkJoin {
229-
name = "evals";
240+
diffs = symlinkJoin {
241+
name = "diffs";
230242
paths = map (
231243
evalSystem:
232-
singleSystem {
233-
inherit quickTest evalSystem chunkSize;
244+
let
245+
eval = singleSystem {
246+
inherit quickTest evalSystem chunkSize;
247+
};
248+
in
249+
diff {
250+
inherit evalSystem;
251+
# Local "full" evaluation doesn't do a real diff.
252+
beforeDir = eval;
253+
afterDir = eval;
234254
}
235255
) evalSystems;
236256
};
237257
in
238258
combine {
239-
evalDir = evals;
259+
diffDir = diffs;
240260
};
241261

242262
in
243263
{
244264
inherit
245265
attrpathsSuperset
246266
singleSystem
267+
diff
247268
combine
248269
compare
249270
# The above three are used by separate VMs in a GitHub workflow,

ci/eval/diff.nix

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
{
2+
lib,
3+
runCommand,
4+
writeText,
5+
}:
6+
7+
{
8+
beforeDir,
9+
afterDir,
10+
evalSystem,
11+
}:
12+
13+
let
14+
/*
15+
Computes the key difference between two attrs
16+
17+
{
18+
added: [ <keys only in the second object> ],
19+
removed: [ <keys only in the first object> ],
20+
changed: [ <keys with different values between the two objects> ],
21+
}
22+
*/
23+
diff =
24+
let
25+
filterKeys = cond: attrs: lib.attrNames (lib.filterAttrs cond attrs);
26+
in
27+
old: new: {
28+
added = filterKeys (n: _: !(old ? ${n})) new;
29+
removed = filterKeys (n: _: !(new ? ${n})) old;
30+
changed = filterKeys (
31+
n: v:
32+
# Filter out attributes that don't exist anymore
33+
(new ? ${n})
34+
35+
# Filter out attributes that are the same as the new value
36+
&& (v != (new.${n}))
37+
) old;
38+
};
39+
40+
getAttrs =
41+
dir:
42+
let
43+
raw = builtins.readFile "${dir}/${evalSystem}/paths.json";
44+
# The file contains Nix paths; we need to ignore them for evaluation purposes,
45+
# else there will be a "is not allowed to refer to a store path" error.
46+
data = builtins.unsafeDiscardStringContext raw;
47+
in
48+
builtins.fromJSON data;
49+
50+
beforeAttrs = getAttrs beforeDir;
51+
afterAttrs = getAttrs afterDir;
52+
diffAttrs = diff beforeAttrs afterAttrs;
53+
diffJson = writeText "diff.json" (builtins.toJSON diffAttrs);
54+
in
55+
runCommand "diff" { } ''
56+
mkdir -p $out/${evalSystem}
57+
58+
cp -r ${beforeDir} $out/before
59+
cp -r ${afterDir} $out/after
60+
cp ${diffJson} $out/${evalSystem}/diff.json
61+
''

0 commit comments

Comments
 (0)