Skip to content

Commit cb5e33f

Browse files
authored
[release] Fix perf metrics compare (#51655)
When doing release benchmark comparison, I found the script is performing comparison on **full filepaths** rather than **release result filenames**. Signed-off-by: dentiny <dentinyhao@gmail.com>
1 parent 447c4f1 commit cb5e33f

File tree

1 file changed

+45
-22
lines changed

1 file changed

+45
-22
lines changed

release/release_logs/compare_perf_metrics

Lines changed: 45 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
11
#!/usr/bin/env python3
2+
"""
3+
This script compares benchmark results from two release directories one by one.
4+
5+
Usage:
6+
python3 release/release_logs/compare_perf_metrics <old-dir> <new-dir>
7+
"""
8+
29
import json
310
import pathlib
411
import argparse
@@ -27,37 +34,43 @@ def parse_args():
2734

2835

2936
def main(old_dir_name, new_dir_name):
37+
old_files = list(walk(old_dir_name))
38+
new_files = list(walk(new_dir_name))
3039

31-
old_paths = paths_without_root_dir(walk(old_dir_name))
32-
new_paths = paths_without_root_dir(walk(new_dir_name))
33-
to_compare, missing_in_new, missing_in_old = get_compare_list(old_paths, new_paths)
40+
old_by_name = group_by_filename(old_files, old_dir_name)
41+
new_by_name = group_by_filename(new_files, new_dir_name)
3442

35-
for path in missing_in_new:
36-
print(new_dir_name, "does not have", path)
37-
38-
for path in missing_in_old:
39-
print(old_dir_name, "does not have", path)
43+
all_filenames = set(old_by_name.keys()) | set(new_by_name.keys())
4044

4145
throughput_regressions = []
4246
latency_regressions = []
4347
missing_in_new = []
4448
missing_in_old = []
45-
for path in to_compare:
46-
old = pathlib.Path(old_dir_name, *path.parts)
47-
new = pathlib.Path(new_dir_name, *path.parts)
4849

49-
throughput, latency, new, old = get_regressions(old, new)
50+
for filename in sorted(all_filenames):
51+
old_path = old_by_name.get(filename)
52+
new_path = new_by_name.get(filename)
53+
54+
if not old_path:
55+
print(f"{old_dir_name} is missing {filename}")
56+
continue
57+
if not new_path:
58+
print(f"{new_dir_name} is missing {filename}")
59+
continue
60+
61+
# Compare the two files
62+
throughput, latency, missing_new_metrics, missing_old_metrics = get_regressions(old_path, new_path)
5063

5164
throughput_regressions.extend(throughput)
5265
latency_regressions.extend(latency)
53-
missing_in_new.extend(new)
54-
missing_in_old.extend(old)
66+
missing_in_new.extend(missing_new_metrics)
67+
missing_in_old.extend(missing_old_metrics)
5568

56-
for perf_metric in missing_in_new:
57-
print(f"{new} does not have {perf_metric}")
69+
for metric in missing_in_new:
70+
print(f"{new_path} does not have {metric}")
5871

59-
for perf_metric in missing_in_old:
60-
print(f"{old} does not have {perf_metric}")
72+
for metric in missing_in_old:
73+
print(f"{old_path} does not have {metric}")
6174

6275
throughput_regressions.sort()
6376
for _, regression in throughput_regressions:
@@ -78,9 +91,20 @@ def walk(dir_name):
7891
stack.extend(root.iterdir())
7992

8093

81-
def paths_without_root_dir(paths):
82-
for p in paths:
83-
yield pathlib.Path(*p.parts[1:])
94+
def group_by_filename(paths, base_dir):
95+
"""
96+
Return a dict mapping filenames to full paths.
97+
If there are duplicates, logging warning and ignore later ones.
98+
"""
99+
file_map = {}
100+
for path in paths:
101+
name = path.name
102+
rel_path = path.relative_to(base_dir)
103+
if name not in file_map:
104+
file_map[name] = path
105+
else:
106+
print(f"Warning: duplicate filename {name} found at {rel_path}")
107+
return file_map
84108

85109

86110
def get_compare_list(old, new):
@@ -93,7 +117,6 @@ def get_compare_list(old, new):
93117
new_set.difference(old_set),
94118
)
95119

96-
97120
def get_regressions(old_path, new_path):
98121

99122
with open(old_path, "r") as f:

0 commit comments

Comments
 (0)