Skip to content

Commit 33f82bb

Browse files
authored
[autorevert] dispatch untargeted signals without tests-to-include filter (#8037)
## Summary Two related fixes to the autorevert lambda's `tests-to-include` dispatch path. Both surface the same anti-pattern: a signal that *cannot* be narrowed to a `run_test.py`-recognized module should be dispatched as a "re-run the whole job(s)" restart, not as a TEST-narrow restart. ## Bug 1 — empty-`file` rows in `tests.all_test_runs` produce bogus `tests-to-include` entries `TestRow.test_id` (`signal_extraction_types.py:138-141`) builds the test key as `f"{file}::{name}" if file else name`. When the CH row has empty `file`, the test key falls back to just the bare `name` (no `::`). `signal_extraction.py` then derived `test_module` from the key: ```python test_module = test_id.split("::")[0].replace(".py", "") ``` With no `::`, `test_module` becomes the bare method name (e.g. `test_partial_eval_graph_conv`). The lambda forwards that into `workflow_dispatch.tests-to-include`, and `pytorch/pytorch:test/run_test.py:144` `TestChoices.__contains__` rejects it with: ``` argument -i/--include: invalid choice: 'test_partial_eval_graph_conv' ``` The dispatched shard fails before running any tests, surfacing as 15-25 spurious `test-osdc` failures per affected commit on HUD trunk. Tracking: pytorch/pytorch-gha-infra#1137. Verified primary-source: `tests.all_test_runs` for `name='test_partial_eval_graph_conv'` over the last 2 days returns three distinct `file` values: `'test_jit.py'`, `'test_jit_legacy.py'`, `''`. Concrete instance — pytorch/pytorch run [25294380825](https://github.com/pytorch/pytorch/actions/runs/25294380825) (workflow_dispatch by `pytorch-auto-revert[bot]`, 2026-05-03 23:51 UTC): ``` jobs-to-include: linux-jammy-py3.10-clang18 tests-to-include: test_jit test_freeze_module_detach_gradient test_partial_eval_graph_conv test_freeze_interface_swapping_two_methods test_partial_eval_stitching test_returning_input_symbolic_shapes ``` `test_jit` is the actual file name; the other five are method names that leaked through the empty-`file` fallback. The `default` / `crossref` / `dynamo_wrapped` shards on this run failed at argparse; the `openreg` / `einops` shards were green only because their `test.sh` paths bypass `$INCLUDE_CLAUSE` entirely. ## Bug 2 — JOB+TEST signals on the same (workflow, commit) silently lose JOB intent `signal_actions.py::group_actions` keys the restart map on `(workflow_name, commit_sha)`. ALL contributing signals — JOB-track and TEST-track — are merged into a single `ActionGroup` with the union of `jobs_to_include` and `tests_to_include`. A JOB-track signal (`test_module=None`, intent = "re-run the whole job") coexisting with a TEST-track signal in the same group becomes a TEST-narrow restart: only the test modules contributed by the TEST signals run, and the JOB signal's full-job-restart intent is silently thrown away. This was a latent bug independent of Bug 1 — surfaced while auditing the `tests-to-include` path. ## Fix 1. `signal_extraction.py`: when `test_id` lacks `::`, set `test_module=None`. The signal stays alive but is marked untargeted. 2. `signal_actions.py::group_actions`: if any source in the group has `test_module is None`, dispatch with `tests_to_include=frozenset()` for the whole group. JOB-track signals (always `test_module=None`) and TEST-track signals with no extractable module both reach the new branch, so they get identical "re-run the whole job(s)" treatment instead of being narrowed by sibling targetable TEST signals. `workflow_checker.py:165` already drops the `tests-to-include` input when the frozenset is empty — no change needed there. ## Test plan - [x] `tests/test_signal_extraction.py` (2 new): empty `file` → Signal with `test_module=None`; populated `file` → existing module path. - [x] `tests/test_signal_actions.py` (4 new in `TestGroupActionsTestsToInclude`): only-targeted-tests keeps filter; TEST-with-no-module drops filter; mixed targeted+untargeted TEST drops filter; JOB+TEST same-(wf,sha) drops filter. - [x] `make test` — all 146 tests pass (1 skipped due to no `GITHUB_TOKEN`). - [x] `ruff format --check` + `ruff check` clean. ## Audit follow-up (not in this PR) Worth checking which test uploader writes empty-`file` rows to `tests.all_test_runs` — likely a partial-export issue from a specific reporter — to fix the data leak at source. Open question carried in `iz2-memory:core/knowledge/autorevert-bot-retry.md`.
1 parent 74ac2cd commit 33f82bb

4 files changed

Lines changed: 256 additions & 6 deletions

File tree

aws/lambda/pytorch-auto-revert/pytorch_auto_revert/signal_actions.py

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -335,16 +335,28 @@ def group_actions(
335335
for (wf, sha), sources in restart_map.items():
336336
jobs = [_derive_job_filter(src.job_base_name) for src in sources]
337337

338+
# If any contributing signal in the group is untargeted at the
339+
# test-module level — i.e. a JOB-track signal (test_module always
340+
# None), or a TEST-track signal whose CH row had empty `file` so
341+
# signal_extraction couldn't derive a module — drop the
342+
# tests-to-include filter for the whole group. Otherwise the
343+
# narrowing contributed by sibling TEST signals would silently
344+
# starve the untargeted signal's "full job re-run" intent.
345+
has_untargeted = any(src.test_module is None for src in sources)
346+
tests_to_include = (
347+
frozenset()
348+
if has_untargeted
349+
else frozenset(src.test_module for src in sources)
350+
)
351+
338352
groups.append(
339353
ActionGroup(
340354
type="restart",
341355
commit_sha=sha,
342356
workflow_target=wf,
343357
sources=sources,
344358
jobs_to_include=frozenset(j for j in jobs if j is not None),
345-
tests_to_include=frozenset(
346-
src.test_module for src in sources if src.test_module
347-
),
359+
tests_to_include=tests_to_include,
348360
)
349361
)
350362
return groups

aws/lambda/pytorch-auto-revert/pytorch_auto_revert/signal_extraction.py

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -478,9 +478,18 @@ def _build_test_signals(
478478
)
479479

480480
if has_any_events:
481-
# Extract test module from test_id (format: "file.py::test_name")
482-
# Result: "file" or "path/to/file" without .py extension
483-
test_module = test_id.split("::")[0].replace(".py", "")
481+
# Extract test module from test_id (format: "file.py::test_name").
482+
# When the CH row's `file` column is empty, TestRow.test_id falls
483+
# back to the bare `name` (no `::`) and we can't derive a path
484+
# that `run_test.py --include` would accept. Mark such signals
485+
# as untargeted (test_module=None) so the action layer dispatches
486+
# them without a tests-to-include filter (job-style restart),
487+
# rather than emitting a bogus method-named module that argparse
488+
# would reject with "invalid choice".
489+
if "::" in test_id:
490+
test_module = test_id.split("::")[0].replace(".py", "")
491+
else:
492+
test_module = None
484493

485494
signals.append(
486495
Signal(

aws/lambda/pytorch-auto-revert/pytorch_auto_revert/tests/test_signal_actions.py

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1271,5 +1271,169 @@ def test_invalid_verdict_string_defaults_to_unsure(self):
12711271
)
12721272

12731273

1274+
class TestGroupActionsTestsToInclude(unittest.TestCase):
1275+
"""Coalescing rules around `tests_to_include` in `group_actions`.
1276+
1277+
Covers the empty-`file` CH-row class (T262...) and the JOB+TEST mixed
1278+
coalescing case where a JOB-track signal's "full job re-run" intent
1279+
must not be silently narrowed by sibling TEST signals.
1280+
"""
1281+
1282+
def _signal(
1283+
self,
1284+
*,
1285+
key: str,
1286+
commit: str,
1287+
source,
1288+
job_base_name: str,
1289+
test_module=None,
1290+
):
1291+
from pytorch_auto_revert.signal import (
1292+
Signal,
1293+
SignalCommit,
1294+
SignalEvent,
1295+
SignalStatus,
1296+
)
1297+
1298+
t0 = datetime(2025, 8, 19, 12, 0, 0)
1299+
c = SignalCommit(
1300+
commit,
1301+
t0,
1302+
[SignalEvent("j", SignalStatus.FAILURE, t0, wf_run_id=1, job_id=1)],
1303+
)
1304+
return Signal(
1305+
key=key,
1306+
workflow_name="trunk",
1307+
commits=[c],
1308+
job_base_name=job_base_name,
1309+
test_module=test_module,
1310+
source=source,
1311+
)
1312+
1313+
def _make_restart(self, commit: str):
1314+
from pytorch_auto_revert.signal import RestartCommits
1315+
1316+
return RestartCommits(commit_shas={commit})
1317+
1318+
def test_only_test_signals_with_modules_keeps_test_filter(self):
1319+
from pytorch_auto_revert.signal import SignalSource
1320+
1321+
proc = SignalActionProcessor()
1322+
s1 = self._signal(
1323+
key="test_jit.py::test_a",
1324+
commit="abc",
1325+
source=SignalSource.TEST,
1326+
job_base_name="linux-jammy-py3.10-clang18 / test",
1327+
test_module="test_jit",
1328+
)
1329+
s2 = self._signal(
1330+
key="test_jit.py::test_b",
1331+
commit="abc",
1332+
source=SignalSource.TEST,
1333+
job_base_name="linux-jammy-py3.10-clang18 / test",
1334+
test_module="test_jit",
1335+
)
1336+
groups = proc.group_actions(
1337+
[(s1, self._make_restart("abc")), (s2, self._make_restart("abc"))]
1338+
)
1339+
restarts = [g for g in groups if g.type == "restart"]
1340+
self.assertEqual(len(restarts), 1)
1341+
g = restarts[0]
1342+
self.assertEqual(g.tests_to_include, frozenset({"test_jit"}))
1343+
self.assertEqual(g.jobs_to_include, frozenset({"linux-jammy-py3.10-clang18"}))
1344+
1345+
def test_test_signal_with_no_module_drops_test_filter(self):
1346+
# TEST signal whose CH row had empty `file` (signal_extraction left
1347+
# test_module=None). Group must dispatch with no tests-to-include —
1348+
# no run_test.py-recognized module to filter on.
1349+
from pytorch_auto_revert.signal import SignalSource
1350+
1351+
proc = SignalActionProcessor()
1352+
s = self._signal(
1353+
key="test_partial_eval_graph_conv",
1354+
commit="abc",
1355+
source=SignalSource.TEST,
1356+
job_base_name="linux-jammy-py3.10-clang18 / test",
1357+
test_module=None,
1358+
)
1359+
groups = proc.group_actions([(s, self._make_restart("abc"))])
1360+
restarts = [g for g in groups if g.type == "restart"]
1361+
self.assertEqual(len(restarts), 1)
1362+
self.assertEqual(restarts[0].tests_to_include, frozenset())
1363+
self.assertEqual(
1364+
restarts[0].jobs_to_include,
1365+
frozenset({"linux-jammy-py3.10-clang18"}),
1366+
)
1367+
1368+
def test_mixed_test_module_and_no_module_drops_test_filter(self):
1369+
# When some sibling TEST signals carry a module and others don't
1370+
# (some CH rows had empty `file`, others didn't — the original
1371+
# bug shape), drop the test filter for the whole group rather
1372+
# than narrowing only the targetable ones and starving the rest.
1373+
from pytorch_auto_revert.signal import SignalSource
1374+
1375+
proc = SignalActionProcessor()
1376+
s_targeted = self._signal(
1377+
key="test_jit.py::test_a",
1378+
commit="abc",
1379+
source=SignalSource.TEST,
1380+
job_base_name="linux-jammy-py3.10-clang18 / test",
1381+
test_module="test_jit",
1382+
)
1383+
s_untargeted = self._signal(
1384+
key="test_partial_eval_graph_conv",
1385+
commit="abc",
1386+
source=SignalSource.TEST,
1387+
job_base_name="linux-jammy-py3.10-clang18 / test",
1388+
test_module=None,
1389+
)
1390+
groups = proc.group_actions(
1391+
[
1392+
(s_targeted, self._make_restart("abc")),
1393+
(s_untargeted, self._make_restart("abc")),
1394+
]
1395+
)
1396+
restarts = [g for g in groups if g.type == "restart"]
1397+
self.assertEqual(len(restarts), 1)
1398+
self.assertEqual(restarts[0].tests_to_include, frozenset())
1399+
1400+
def test_job_track_and_test_track_same_workflow_drops_test_filter(self):
1401+
# Mixed JOB-track + TEST-track signals on the same (workflow, sha).
1402+
# JOB signal wants the entire job re-run; coalescing previously
1403+
# narrowed the dispatch to TEST signals' modules, throwing the JOB
1404+
# signal's intent away. New rule: any untargeted source → empty
1405+
# tests_to_include.
1406+
from pytorch_auto_revert.signal import SignalSource
1407+
1408+
proc = SignalActionProcessor()
1409+
s_job = self._signal(
1410+
key="linux-jammy-py3.10-clang18 / build",
1411+
commit="abc",
1412+
source=SignalSource.JOB,
1413+
job_base_name="linux-jammy-py3.10-clang18 / build",
1414+
test_module=None,
1415+
)
1416+
s_test = self._signal(
1417+
key="test_jit.py::test_a",
1418+
commit="abc",
1419+
source=SignalSource.TEST,
1420+
job_base_name="linux-jammy-py3.10-clang18 / test",
1421+
test_module="test_jit",
1422+
)
1423+
groups = proc.group_actions(
1424+
[(s_job, self._make_restart("abc")), (s_test, self._make_restart("abc"))]
1425+
)
1426+
restarts = [g for g in groups if g.type == "restart"]
1427+
self.assertEqual(len(restarts), 1)
1428+
g = restarts[0]
1429+
self.assertEqual(g.tests_to_include, frozenset())
1430+
self.assertEqual(
1431+
g.jobs_to_include,
1432+
frozenset(
1433+
{"linux-jammy-py3.10-clang18"}
1434+
), # both job_base_names normalize to same display name
1435+
)
1436+
1437+
12741438
if __name__ == "__main__":
12751439
unittest.main()

aws/lambda/pytorch-auto-revert/pytorch_auto_revert/tests/test_signal_extraction.py

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1094,6 +1094,71 @@ def test_advisor_verdict_on_test_signal(self):
10941094
self.assertEqual(c2.advisor_result.verdict.value, "garbage")
10951095
self.assertEqual(c2.advisor_result.signal_key, test_key)
10961096

1097+
def test_test_signal_with_empty_file_has_no_test_module(self):
1098+
# When tests.all_test_runs has empty `file` for a row, TestRow.test_id
1099+
# falls back to the bare `name` (no `::`). signal_extraction must mark
1100+
# the resulting Signal as untargeted (test_module=None) instead of
1101+
# emitting a bogus method-named module that run_test.py --include
1102+
# would later reject with "invalid choice".
1103+
jobs = [
1104+
J(
1105+
sha="C1",
1106+
run=900,
1107+
job=900,
1108+
attempt=1,
1109+
started_at=ts(self.t0, 1),
1110+
conclusion="failure",
1111+
rule="pytest failure",
1112+
)
1113+
]
1114+
tests = [
1115+
T(
1116+
job=900,
1117+
run=900,
1118+
attempt=1,
1119+
file="", # CH row with no file path — primary failure mode
1120+
name="test_partial_eval_graph_conv",
1121+
failure_runs=1,
1122+
success_runs=0,
1123+
)
1124+
]
1125+
signals = self._extract(jobs, tests)
1126+
sig = self._find_test_signal(signals, "trunk", "test_partial_eval_graph_conv")
1127+
self.assertIsNotNone(sig)
1128+
self.assertIsNone(sig.test_module)
1129+
1130+
def test_test_signal_with_populated_file_has_test_module(self):
1131+
# Sanity: the normal `file::name` path still produces a usable
1132+
# test_module (`test_jit` from `test_jit.py::...`).
1133+
jobs = [
1134+
J(
1135+
sha="C1",
1136+
run=901,
1137+
job=901,
1138+
attempt=1,
1139+
started_at=ts(self.t0, 1),
1140+
conclusion="failure",
1141+
rule="pytest failure",
1142+
)
1143+
]
1144+
tests = [
1145+
T(
1146+
job=901,
1147+
run=901,
1148+
attempt=1,
1149+
file="test_jit.py",
1150+
name="test_partial_eval_graph_conv",
1151+
failure_runs=1,
1152+
success_runs=0,
1153+
)
1154+
]
1155+
signals = self._extract(jobs, tests)
1156+
sig = self._find_test_signal(
1157+
signals, "trunk", "test_jit.py::test_partial_eval_graph_conv"
1158+
)
1159+
self.assertIsNotNone(sig)
1160+
self.assertEqual(sig.test_module, "test_jit")
1161+
10971162

10981163
if __name__ == "__main__":
10991164
unittest.main()

0 commit comments

Comments
 (0)