Skip to content

Commit 3c603af

Browse files
authored
Fix release runner group reconcile: per-ref discovery + _select-release-runner callers (#8387)
Follow-up after the pytorch/pytorch stack that refactored runner selection into `_select-release-runner.yml` (pytorch/pytorch#190619). Fixes two bugs in the reconcile automation (#8332) that only hit the apply path, so PR CI (dry-run) stayed green. Found and verified via an end-to-end apply test against the prod runner groups. 1. **select-runner callers dropped from the allow-list** — discovery only matched inline `rel-` labels. After the refactor, `docker-release.yml`, `build-manywheel-images.yml`, and `build-almalinux-images.yml` get their runner from `_select-release-runner.yml` outputs and were excluded. Discovery now propagates the release-label signal up the `uses:` graph (a workflow is an entry if it references a `rel-` label *or* invokes a local reusable that does), keeping the module's "discovered, not hardcoded" design. 2. **Allow-list was a main-only cross-product** — desired = (workflows discovered on `main`) × (all target refs). But release branches diverge: a workflow on `main` may be absent on an older release branch, and a workflow may use release runners on a release branch but not on `main` (e.g. `docker-builds.yml` on `release/2.13`). GitHub rejects the whole PATCH if any `selected_workflows` entry doesn't exist at its ref. Discovery is now **per-ref**, so the allow-list only references workflows that actually exist and run on release runners at each ref. Also surfaces the GitHub API response body on HTTP errors — a swallowed 400 body is what obscured bug #2 during debugging. ## Testing - Unit tests pass (`python3 -m unittest tools.tests.test_release_manage_runner_groups`). - **End-to-end apply** reconciled all 5 prod release runner groups to the correct 19-entry state (`main`: 9, `nightly`: 9, `release/2.13`: 1, `release/2.12`: 0) and is idempotent on re-run.
1 parent cc1468d commit 3c603af

2 files changed

Lines changed: 147 additions & 36 deletions

File tree

tools/scripts/release_manage_runner_groups.py

Lines changed: 80 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,14 @@ def request(self, method: str, path: str, **kwargs: Any) -> "requests.Response":
8181
time.sleep(delay)
8282
continue
8383
break
84-
resp.raise_for_status()
84+
try:
85+
resp.raise_for_status()
86+
except requests.HTTPError as error:
87+
# Surface the API response body; GitHub explains which field was
88+
# rejected there, and raise_for_status() would otherwise drop it.
89+
raise requests.HTTPError(
90+
f"{error} - body: {resp.text}", response=resp
91+
) from error
8592
return resp
8693

8794
def paginate(
@@ -154,6 +161,24 @@ def uses_release_label(text: str) -> bool:
154161
return RELEASE_LABEL_RE.search(text) is not None
155162

156163

164+
def local_uses(job: Any) -> Optional[str]:
165+
"""The local (``./``) reusable-workflow path a job invokes, if any."""
166+
if not isinstance(job, dict):
167+
return None
168+
uses = job.get("uses")
169+
if isinstance(uses, str) and uses.startswith("./"):
170+
return uses.split("@", 1)[0].removeprefix("./")
171+
return None
172+
173+
174+
def local_uses_paths(wf: "WorkflowFile") -> Set[str]:
175+
"""The local reusable-workflow paths invoked by any of a workflow's jobs."""
176+
jobs = wf.doc.get("jobs")
177+
if not isinstance(jobs, dict):
178+
return set()
179+
return {local for local in map(local_uses, jobs.values()) if local is not None}
180+
181+
157182
@dataclass
158183
class WorkflowFile:
159184
doc: Dict[str, Any]
@@ -181,10 +206,13 @@ class WorkflowFile:
181206
"""
182207

183208

184-
def fetch_workflow_files(client: GitHubClient) -> Dict[str, WorkflowFile]:
185-
# Fetch every workflow file's content in a single GraphQL request. Fetching
186-
# each file over its raw.githubusercontent.com download_url instead gets
187-
# rate-limited (HTTP 429) on repos with many workflows like pytorch/pytorch.
209+
def fetch_workflow_files(
210+
client: GitHubClient, rev: str = "main"
211+
) -> Dict[str, WorkflowFile]:
212+
# Fetch every workflow file's content at ``rev`` in a single GraphQL request.
213+
# Fetching each file over its raw.githubusercontent.com download_url instead
214+
# gets rate-limited (HTTP 429) on repos with many workflows like
215+
# pytorch/pytorch.
188216
owner, name = TARGET_REPO.split("/")
189217
resp = client.request(
190218
"POST",
@@ -194,7 +222,7 @@ def fetch_workflow_files(client: GitHubClient) -> Dict[str, WorkflowFile]:
194222
"variables": {
195223
"owner": owner,
196224
"name": name,
197-
"expression": f"main:{WORKFLOWS_DIR}",
225+
"expression": f"{rev}:{WORKFLOWS_DIR}",
198226
},
199227
},
200228
).json()
@@ -222,12 +250,21 @@ def fetch_workflow_files(client: GitHubClient) -> Dict[str, WorkflowFile]:
222250
def collect_release_workflow_paths(files: Dict[str, WorkflowFile]) -> Set[str]:
223251
"""Discover the workflows that run on the release runner labels.
224252
225-
Entry workflows reference a release label directly. From each, follow local
226-
``uses:`` references, but only for jobs that themselves run on a release
227-
label, so the build reusable is included while sibling test/upload jobs
228-
(which run on other runners) are not.
253+
An entry workflow either references a release label directly, or invokes a
254+
local reusable that does (pytorch/pytorch#190619 centralized the labels into
255+
``_select-release-runner.yml``, whose callers get their runner from its
256+
outputs and so carry no label of their own) -- the release-label signal is
257+
propagated up the ``uses:`` graph rather than matching a hardcoded filename.
258+
From each entry, follow local ``uses:`` references, but only for jobs that
259+
themselves run on a release label, so the build reusable is included while
260+
sibling test/upload jobs (which run on other runners) are not.
229261
"""
230-
entry_paths = {path for path, wf in files.items() if uses_release_label(wf.raw)}
262+
label_files = {path for path, wf in files.items() if uses_release_label(wf.raw)}
263+
entry_paths = {
264+
path
265+
for path, wf in files.items()
266+
if path in label_files or local_uses_paths(wf) & label_files
267+
}
231268
seen: Set[str] = set()
232269
queue = list(entry_paths)
233270
while queue:
@@ -242,31 +279,43 @@ def collect_release_workflow_paths(files: Dict[str, WorkflowFile]) -> Set[str]:
242279
if not isinstance(jobs, dict):
243280
continue
244281
for job in jobs.values():
245-
if not isinstance(job, dict):
246-
continue
247-
uses = job.get("uses")
248-
if not (isinstance(uses, str) and uses.startswith("./")):
282+
local = local_uses(job)
283+
if local is None:
249284
continue
250285
if not uses_release_label(str(job)):
251286
continue
252-
local = uses.split("@", 1)[0][len("./") :]
253-
if local not in seen:
254-
queue.append(local)
287+
queue.append(local)
255288
return seen
256289

257290

258-
def discover_release_workflows(client: GitHubClient) -> Set[str]:
259-
files = fetch_workflow_files(client)
260-
paths = collect_release_workflow_paths(files)
261-
log(f"Discovered {len(paths)} release workflow(s) on {TARGET_REPO}@main:")
262-
for path in sorted(paths):
263-
log(f" {path}")
264-
return paths
265-
291+
def discover_release_workflows(
292+
client: GitHubClient, refs: Iterable[str]
293+
) -> Dict[str, Set[str]]:
294+
"""Discover release workflows independently at each target ref.
266295
267-
def build_desired_workflows(paths: Iterable[str], refs: Iterable[str]) -> Set[str]:
268-
refs = list(refs)
269-
return {f"{TARGET_REPO}/{path}@{ref}" for path in paths for ref in refs}
296+
Refs diverge: a workflow present on ``main`` (e.g. the newly added
297+
``_select-release-runner.yml``) may be absent on an older release branch, and
298+
GitHub rejects the whole allow-list PATCH if any ``selected_workflows`` entry
299+
does not exist at its ref. So discovery is per-ref rather than a main-only
300+
scan cross-producted onto every ref.
301+
"""
302+
paths_by_ref: Dict[str, Set[str]] = {}
303+
for ref in refs:
304+
rev = ref.removeprefix("refs/heads/")
305+
paths = collect_release_workflow_paths(fetch_workflow_files(client, rev))
306+
log(f"Discovered {len(paths)} release workflow(s) on {TARGET_REPO}@{rev}:")
307+
for path in sorted(paths):
308+
log(f" {path}")
309+
paths_by_ref[ref] = paths
310+
return paths_by_ref
311+
312+
313+
def build_desired_workflows(paths_by_ref: Dict[str, Set[str]]) -> Set[str]:
314+
return {
315+
f"{TARGET_REPO}/{path}@{ref}"
316+
for ref, paths in paths_by_ref.items()
317+
for path in paths
318+
}
270319

271320

272321
# --- Runner group reconciliation -------------------------------------------
@@ -376,8 +425,8 @@ def main() -> None:
376425

377426
refs = get_target_refs(client)
378427
log(f"Target refs: {refs}")
379-
paths = discover_release_workflows(client)
380-
desired = build_desired_workflows(paths, refs)
428+
paths_by_ref = discover_release_workflows(client, refs)
429+
desired = build_desired_workflows(paths_by_ref)
381430
log(f"Desired allow-list ({len(desired)} references):")
382431
for entry in sorted(desired):
383432
log(f" {entry}")

tools/tests/test_release_manage_runner_groups.py

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,61 @@ def test_entry_without_reusable_returns_just_itself(self) -> None:
153153
{".github/workflows/build-vllm-wheel.yml"},
154154
)
155155

156+
def test_discovers_callers_of_select_release_runner(self) -> None:
157+
# pytorch/pytorch#190619 moved the rel- labels into
158+
# _select-release-runner.yml, which emits them as outputs. Callers carry
159+
# no rel- label of their own (runs-on references needs.<job>.outputs.*),
160+
# so they must be discovered via their use of the select-runner reusable.
161+
files = {
162+
".github/workflows/docker-release.yml": m.WorkflowFile(
163+
doc={
164+
"jobs": {
165+
"select-runner": {
166+
"uses": "./.github/workflows/_select-release-runner.yml"
167+
},
168+
"build": {"runs-on": "${{ matrix.runner }}"},
169+
}
170+
},
171+
raw="uses: ./.github/workflows/_select-release-runner.yml\n"
172+
"runs-on: ${{ matrix.runner }}",
173+
),
174+
".github/workflows/_select-release-runner.yml": m.WorkflowFile(
175+
doc={"jobs": {"select": {"runs-on": "ubuntu-24.04"}}},
176+
raw="echo x86=mt-rel-l-x86iavx512-44-340",
177+
),
178+
".github/workflows/unrelated.yml": m.WorkflowFile(
179+
doc={"jobs": {"build": {"runs-on": "ubuntu-24.04"}}},
180+
raw="runs-on: ubuntu-24.04",
181+
),
182+
}
183+
self.assertEqual(
184+
m.collect_release_workflow_paths(files),
185+
{
186+
".github/workflows/docker-release.yml",
187+
".github/workflows/_select-release-runner.yml",
188+
},
189+
)
190+
191+
def test_paths_filter_mention_is_not_a_caller(self) -> None:
192+
# Even when the release-label reusable is present, a workflow that only
193+
# names it in a triggers paths: filter (not an actual job `uses:`) must
194+
# not be pulled in -- entry detection is by job `uses:`, not raw text.
195+
files = {
196+
".github/workflows/_select-release-runner.yml": m.WorkflowFile(
197+
doc={"jobs": {"select": {"runs-on": "ubuntu-24.04"}}},
198+
raw="echo x86=mt-rel-l-x86iavx512-44-340",
199+
),
200+
".github/workflows/lint.yml": m.WorkflowFile(
201+
doc={"jobs": {"lint": {"runs-on": "ubuntu-24.04"}}},
202+
raw="on:\n pull_request:\n paths:\n"
203+
" - .github/workflows/_select-release-runner.yml",
204+
),
205+
}
206+
self.assertEqual(
207+
m.collect_release_workflow_paths(files),
208+
{".github/workflows/_select-release-runner.yml"},
209+
)
210+
156211
def test_ignores_remote_uses(self) -> None:
157212
files = {
158213
".github/workflows/gen.yml": m.WorkflowFile(
@@ -173,18 +228,25 @@ def test_ignores_remote_uses(self) -> None:
173228

174229

175230
class TestBuildDesiredWorkflows(TestCase):
176-
def test_is_cross_product(self) -> None:
231+
def test_flattens_per_ref_paths(self) -> None:
232+
# Per-ref discovery: each ref carries only the workflows that exist there,
233+
# so a ref (release/2.12) can legitimately omit a workflow (b.yml) that
234+
# main has. The allow-list must not fabricate the missing combination.
177235
desired = m.build_desired_workflows(
178-
[".github/workflows/a.yml", ".github/workflows/b.yml"],
179-
["refs/heads/main", "refs/heads/nightly"],
236+
{
237+
"refs/heads/main": {
238+
".github/workflows/a.yml",
239+
".github/workflows/b.yml",
240+
},
241+
"refs/heads/release/2.12": {".github/workflows/a.yml"},
242+
}
180243
)
181244
self.assertEqual(
182245
desired,
183246
{
184247
"pytorch/pytorch/.github/workflows/a.yml@refs/heads/main",
185-
"pytorch/pytorch/.github/workflows/a.yml@refs/heads/nightly",
186248
"pytorch/pytorch/.github/workflows/b.yml@refs/heads/main",
187-
"pytorch/pytorch/.github/workflows/b.yml@refs/heads/nightly",
249+
"pytorch/pytorch/.github/workflows/a.yml@refs/heads/release/2.12",
188250
},
189251
)
190252

0 commit comments

Comments
 (0)