Skip to content

Commit 8aa8a37

Browse files
authored
Avoid overwriting the default test times if the new default is empty (#6289)
Due to the recent issue with octokit/request-action#315, the workflow to upload test stats was broken for a few days. The issue has been mitigated by pytorch/pytorch#146940, but we still have a gap in test times until new stats come in. The test times from the last 3 viable/strict commits were used to compute test times https://github.com/pytorch/test-infra/blob/main/torchci/clickhouse_queries/test_time_per_file/query.sql#L11-L12 The `tools/torchci/update_test_times.py` script had a bug in which it overwrote the default test times even when there was no stats from the above query. The effect of this was that [run_test](https://github.com/pytorch/pytorch/blob/main/test/run_test.py#L1909-L1936) had no default values to use in some cases, forcing these jobs to run sequentially, for example: * https://github.com/pytorch/pytorch/actions/runs/13295462436/job/37129445091#step:22:670 * https://github.com/pytorch/pytorch/actions/runs/13297960390/job/37134948183#step:8:100 (Notice that they all had just 1/1 shard) The proposed fix here is to keep the old default values when there is no new stats. We could also, maybe, create a metric for this so that we can monitor the age of the default test times.
1 parent d0db069 commit 8aa8a37

File tree

2 files changed

+28
-10
lines changed

2 files changed

+28
-10
lines changed

tools/torchci/tests/test_update_test_times.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,12 @@ def test_gen_test_file_times_override_old_default(self) -> None:
8080
}
8181
self.assertDictEqual(res, expected)
8282

83+
data = []
84+
res = gen_test_file_times(data, {"default": {"config": {"a": 57}}})
85+
# When having no data, the old default should be kept
86+
expected = {"default": {"config": {"a": 57}}}
87+
self.assertDictEqual(res, expected)
88+
8389
def test_gen_test_file_times_old_values_still_present(self) -> None:
8490
data = [
8591
self.make_db_row("env", "config", "a", 5),

tools/torchci/update_test_times.py

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,17 @@ def gen_test_file_times(db_results, old_test_times):
9797
for test, times in test_times_no_test_config.items():
9898
test_times_no_test_config[test] = sum(times) / len(times)
9999

100-
# Default should never be a build env
101-
test_times["default"] = test_times_no_build_env
102-
# Replace default's default with our own to account for tests that aren't
103-
# usually in the default test config like distributed
104-
test_times["default"]["default"] = test_times_no_test_config
100+
# Avoid overwriting the default if the new default is empty
101+
if test_times_no_build_env:
102+
# Default should never be a build env
103+
test_times["default"] = test_times_no_build_env
104+
105+
# Avoid overwriting the default if the new default is empty
106+
if test_times_no_test_config:
107+
# Replace default's default with our own to account for tests that aren't
108+
# usually in the default test config like distributed
109+
test_times["default"]["default"] = test_times_no_test_config
110+
105111
return test_times
106112

107113

@@ -136,11 +142,17 @@ def gen_test_class_times(db_results, old_test_times):
136142
for testclass, times in test_times_no_test_config[file].items():
137143
test_times_no_test_config[file][testclass] = sum(times) / len(times)
138144

139-
# Default should never be a build env
140-
test_times["default"] = test_times_no_build_env
141-
# Replace default's default with our own to account for tests that aren't
142-
# usually in the default test config like distributed
143-
test_times["default"]["default"] = test_times_no_test_config
145+
# Avoid overwriting the default if the new default is empty
146+
if test_times_no_build_env:
147+
# Default should never be a build env
148+
test_times["default"] = test_times_no_build_env
149+
150+
# Avoid overwriting the default if the new default is empty
151+
if test_times_no_test_config:
152+
# Replace default's default with our own to account for tests that aren't
153+
# usually in the default test config like distributed
154+
test_times["default"]["default"] = test_times_no_test_config
155+
144156
return test_times
145157

146158

0 commit comments

Comments
 (0)