Skip to content

Commit a3dc1bb

Browse files
jeanschmidtjeanschmidt
authored
Serve Dr. CI's fetchPR from ClickHouse with a GitHub fallback (R1) (#8576)
Stack from [ghstack](https://github.com/ezyang/ghstack/tree/0.14.0) (oldest at bottom): * #8584 * #8582 * #8580 * #8579 * #8578 * __->__ #8576 **Impact:** Dr. CI / drci refreshes and the `/pull` page (torchci) **Risk:** low ## What `fetchPR` now reads a PR's title, body, and commit list from the ClickHouse mirrors (`default.pull_request` + `pr_commits`) instead of the GitHub REST API, and only calls GitHub when ClickHouse misses, errors, or is stale. ## Why The PyTorchBot GitHub App installation keeps hitting its shared hourly rate limit and returning 403s. `fetchPR` runs unconditionally once per PR per Dr. CI run and was one of the biggest steady drains (~600 calls/hr). Key behaviours to check while reviewing: - Both ClickHouse reads run under `Promise.allSettled`; each independently falls back to GitHub on an empty result **or** on error, so a ClickHouse outage degrades to today's GitHub-only path rather than failing the refresh. - The `listCommits` call is skipped only when our commit list is non-empty *and* its newest sha matches a known head sha (caller-supplied by Dr. CI, else the head sha from the ClickHouse row). Fork PRs return empty from `pr_commits`, so they naturally fall back to GitHub. - Dr. CI passes `workflows[0].head_sha` as the reference head; the `/pull` page passes nothing and relies on the ClickHouse head sha. # Notes GitHub is still the source of truth for anything the mirror can't cover (fork PRs, ~31% of older PRs not mirrored, and the ~25–56s ingest lag on very recent pushes). New `fetchPR.test.ts` covers the hit / miss / error / stale-tip paths. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
1 parent 5232da6 commit a3dc1bb

3 files changed

Lines changed: 283 additions & 16 deletions

File tree

‎torchci/lib/fetchPR.ts‎

Lines changed: 109 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { Octokit } from "octokit";
2-
import { queryClickhouseSaved } from "./clickhouse";
2+
import { queryClickhouse, queryClickhouseSaved } from "./clickhouse";
33
import { PRData } from "./types";
44

55
async function fetchHistoricalCommits(
@@ -14,37 +14,131 @@ async function fetchHistoricalCommits(
1414
});
1515
}
1616

17+
interface PRTitleBody {
18+
title: string;
19+
body: string;
20+
headSha: string;
21+
}
22+
23+
async function fetchPRTitleBody(
24+
owner: string,
25+
repo: string,
26+
prNumber: string
27+
): Promise<PRTitleBody | undefined> {
28+
// Read the PR's title/body/head sha from the default.pull_request mirror
29+
// instead of the GitHub API. Filter on `number` (the table's sorting key) for
30+
// an indexed lookup; html_url pins the repo since PR numbers are not unique
31+
// across repos. FINAL collapses the ReplacingMergeTree to the latest row.
32+
const query = `
33+
SELECT
34+
title,
35+
body,
36+
head.'sha' AS head_sha
37+
FROM default.pull_request FINAL
38+
WHERE
39+
number = {prNumber: Int64}
40+
AND html_url = {htmlUrl: String}
41+
`;
42+
const rows = await queryClickhouse(query, {
43+
prNumber,
44+
htmlUrl: `https://github.com/${owner}/${repo}/pull/${prNumber}`,
45+
});
46+
if (rows.length !== 1) {
47+
return undefined;
48+
}
49+
return {
50+
title: rows[0].title,
51+
body: rows[0].body ?? "",
52+
headSha: rows[0].head_sha,
53+
};
54+
}
55+
1756
export default async function fetchPR(
1857
owner: string,
1958
repo: string,
2059
prNumber: string,
21-
octokit: Octokit
60+
octokit: Octokit,
61+
knownHeadSha?: string
2262
): Promise<PRData> {
2363
// We pull data from both our database and Github to get all commits,
2464
// including the ones that have been force merged out of the git history. Our
2565
// database is the primary source, GitHub covers anything newer that might
2666
// have been missed.
27-
const [pull, commits, historicalCommits] = await Promise.all([
28-
octokit.rest.pulls.get({
29-
owner,
30-
repo,
31-
pull_number: parseInt(prNumber),
32-
}),
33-
octokit.paginate(octokit.rest.pulls.listCommits, {
67+
//
68+
// Both ClickHouse reads run in parallel so a covered PR resolves in a single
69+
// round trip and never touches the GitHub REST API. Each read independently
70+
// falls back to GitHub on an empty result OR on error, so a ClickHouse outage
71+
// degrades to GitHub-only behaviour instead of failing the refresh.
72+
const [titleBodySettled, historicalCommitsSettled] = await Promise.allSettled(
73+
[
74+
fetchPRTitleBody(owner, repo, prNumber),
75+
fetchHistoricalCommits(owner, repo, prNumber),
76+
]
77+
);
78+
79+
let titleBody: PRTitleBody | undefined;
80+
if (titleBodySettled.status === "fulfilled") {
81+
titleBody = titleBodySettled.value;
82+
} else {
83+
console.warn(
84+
`fetchPR: ClickHouse title/body query failed for ${owner}/${repo}#${prNumber}, falling back to GitHub`,
85+
titleBodySettled.reason
86+
);
87+
}
88+
89+
let title: string;
90+
let body: string;
91+
if (titleBody !== undefined) {
92+
title = titleBody.title;
93+
body = titleBody.body;
94+
} else {
95+
// No ClickHouse row (or the query errored): fall back to the GitHub API.
96+
const pull = await octokit.rest.pulls.get({
3497
owner,
3598
repo,
3699
pull_number: parseInt(prNumber),
37-
per_page: 100,
38-
}),
39-
fetchHistoricalCommits(owner, repo, prNumber),
40-
]);
41-
const title = pull.data.title;
42-
const body = pull.data.body ?? "";
100+
});
101+
title = pull.data.title;
102+
body = pull.data.body ?? "";
103+
}
104+
105+
let historicalCommits: any[] = [];
106+
if (historicalCommitsSettled.status === "fulfilled") {
107+
historicalCommits = historicalCommitsSettled.value;
108+
} else {
109+
console.warn(
110+
`fetchPR: ClickHouse pr_commits query failed for ${owner}/${repo}#${prNumber}, falling back to GitHub`,
111+
historicalCommitsSettled.reason
112+
);
113+
}
43114

44115
let shas = historicalCommits.map((commit) => {
45116
return { sha: commit.sha, title: commit.message.split("\n")[0] };
46117
});
47118

119+
// The reference head sha is the caller-supplied head (Dr. CI already knows it)
120+
// or, failing that, the head sha from the ClickHouse pull_request row. When it
121+
// is undefined (e.g. the /pull page with a ClickHouse miss) we can't prove our
122+
// commit list is current, so we fall through to GitHub exactly like before.
123+
const referenceHeadSha = knownHeadSha ?? titleBody?.headSha;
124+
const newestHistoricalSha =
125+
shas.length > 0 ? shas[shas.length - 1].sha : undefined;
126+
127+
// Skip the GitHub listCommits call when our database already has the tip:
128+
// there are commits AND the newest one matches the known PR head. Otherwise
129+
// (empty list, or newest sha differs) hit GitHub and reconcile below. Fork PRs
130+
// return empty from pr_commits, so they naturally fall back.
131+
if (shas.length !== 0 && newestHistoricalSha === referenceHeadSha) {
132+
return { title, body, shas };
133+
}
134+
135+
const commits = await octokit.paginate(octokit.rest.pulls.listCommits, {
136+
owner,
137+
repo,
138+
pull_number: parseInt(prNumber),
139+
per_page: 100,
140+
});
141+
48142
// Ideally historicalCommits will be a superset of commits, but if there's a propagation delay with
49143
// getting the data to our database it may be missing recent commits for a bit.
50144
if (shas.length === 0) {

‎torchci/pages/api/drci/drci.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1448,7 +1448,13 @@ export async function reorganizeWorkflows(
14481448
let prShas: { sha: string; title: string }[] = [];
14491449
// Gate this to PyTorch as disabled tests feature is only available there
14501450
if (octokit && repo === "pytorch") {
1451-
const prData = await fetchPR(owner, repo, `${prNumber}`, octokit);
1451+
const prData = await fetchPR(
1452+
owner,
1453+
repo,
1454+
`${prNumber}`,
1455+
octokit,
1456+
workflows[0].head_sha
1457+
);
14521458
prTitle = prData.title;
14531459
prBody = prData.body;
14541460
prShas = prData.shas;

‎torchci/test/fetchPR.test.ts‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
import * as clickhouse from "lib/clickhouse";
2+
import fetchPR from "lib/fetchPR";
3+
import { Octokit } from "octokit";
4+
5+
function makeOctokit(opts: { getResult?: any; paginateResult?: any[] }) {
6+
const get = jest.fn().mockResolvedValue(opts.getResult ?? { data: {} });
7+
// listCommits is only passed to paginate as an endpoint reference; the fake
8+
// paginate ignores it, so it is never actually invoked by fetchPR.
9+
const listCommits = jest.fn();
10+
const paginate = jest.fn().mockResolvedValue(opts.paginateResult ?? []);
11+
const octokit = {
12+
rest: { pulls: { get, listCommits } },
13+
paginate,
14+
} as unknown as Octokit;
15+
return { octokit, get, listCommits, paginate };
16+
}
17+
18+
describe("fetchPR", () => {
19+
let queryClickhouse: jest.SpyInstance;
20+
let queryClickhouseSaved: jest.SpyInstance;
21+
22+
beforeEach(() => {
23+
queryClickhouse = jest.spyOn(clickhouse, "queryClickhouse");
24+
queryClickhouseSaved = jest.spyOn(clickhouse, "queryClickhouseSaved");
25+
});
26+
27+
afterEach(() => {
28+
jest.restoreAllMocks();
29+
});
30+
31+
test("(a) ClickHouse hit uses CH title/body and does not call pulls.get", async () => {
32+
queryClickhouse.mockResolvedValue([
33+
{ title: "CH title", body: "CH body", head_sha: "shaA" },
34+
]);
35+
queryClickhouseSaved.mockResolvedValue([
36+
{ sha: "shaA", message: "commit a\nsecond line" },
37+
]);
38+
const { octokit, get, paginate } = makeOctokit({});
39+
40+
const result = await fetchPR("pytorch", "pytorch", "123", octokit);
41+
42+
expect(result).toEqual({
43+
title: "CH title",
44+
body: "CH body",
45+
shas: [{ sha: "shaA", title: "commit a" }],
46+
});
47+
expect(get).not.toHaveBeenCalled();
48+
// CH already has the tip (newest sha === head sha), so no GitHub commits call.
49+
expect(paginate).not.toHaveBeenCalled();
50+
// Title/body query is pinned by the exact html_url + PR number.
51+
expect(queryClickhouse.mock.calls[0][1]).toEqual({
52+
prNumber: "123",
53+
htmlUrl: "https://github.com/pytorch/pytorch/pull/123",
54+
});
55+
});
56+
57+
test("(b) ClickHouse miss (empty) falls back to pulls.get", async () => {
58+
queryClickhouse.mockResolvedValue([]);
59+
queryClickhouseSaved.mockResolvedValue([]);
60+
const { octokit, get, paginate } = makeOctokit({
61+
getResult: {
62+
data: { title: "GH title", body: "GH body", head: { sha: "shaGH" } },
63+
},
64+
paginateResult: [{ sha: "shaGH", commit: { message: "gh commit" } }],
65+
});
66+
67+
const result = await fetchPR("pytorch", "pytorch", "123", octokit);
68+
69+
expect(result).toEqual({
70+
title: "GH title",
71+
body: "GH body",
72+
shas: [{ sha: "shaGH", title: "gh commit" }],
73+
});
74+
expect(get).toHaveBeenCalledTimes(1);
75+
// No CH commits at all, so GitHub commits are fetched.
76+
expect(paginate).toHaveBeenCalledTimes(1);
77+
});
78+
79+
test("(c) ClickHouse error falls back to GitHub without crashing", async () => {
80+
const warn = jest.spyOn(console, "warn").mockImplementation(() => {});
81+
queryClickhouse.mockRejectedValue(new Error("clickhouse down"));
82+
queryClickhouseSaved.mockRejectedValue(new Error("clickhouse down"));
83+
const { octokit, get, paginate } = makeOctokit({
84+
getResult: {
85+
data: { title: "GH title", body: "GH body", head: { sha: "shaGH" } },
86+
},
87+
paginateResult: [{ sha: "shaGH", commit: { message: "gh commit" } }],
88+
});
89+
90+
const result = await fetchPR("pytorch", "pytorch", "123", octokit);
91+
92+
expect(result).toEqual({
93+
title: "GH title",
94+
body: "GH body",
95+
shas: [{ sha: "shaGH", title: "gh commit" }],
96+
});
97+
expect(get).toHaveBeenCalledTimes(1);
98+
expect(paginate).toHaveBeenCalledTimes(1);
99+
// Both failing reads are logged with detail, not silently swallowed.
100+
expect(warn).toHaveBeenCalledTimes(2);
101+
});
102+
103+
test("(d) newest CH sha === knownHeadSha skips listCommits", async () => {
104+
queryClickhouse.mockResolvedValue([
105+
{ title: "CH title", body: "CH body", head_sha: "ignored" },
106+
]);
107+
queryClickhouseSaved.mockResolvedValue([
108+
{ sha: "old", message: "m1" },
109+
{ sha: "shaHead", message: "tip" },
110+
]);
111+
const { octokit, get, paginate } = makeOctokit({});
112+
113+
const result = await fetchPR(
114+
"pytorch",
115+
"pytorch",
116+
"123",
117+
octokit,
118+
"shaHead"
119+
);
120+
121+
expect(result).toEqual({
122+
title: "CH title",
123+
body: "CH body",
124+
shas: [
125+
{ sha: "old", title: "m1" },
126+
{ sha: "shaHead", title: "tip" },
127+
],
128+
});
129+
expect(get).not.toHaveBeenCalled();
130+
expect(paginate).not.toHaveBeenCalled();
131+
});
132+
133+
test("(e) newest CH sha !== knownHeadSha calls listCommits and reconciles the tip", async () => {
134+
queryClickhouse.mockResolvedValue([
135+
{ title: "CH title", body: "CH body", head_sha: "ignored" },
136+
]);
137+
queryClickhouseSaved.mockResolvedValue([
138+
{ sha: "old1", message: "m1" },
139+
{ sha: "old2", message: "m2" },
140+
]);
141+
const { octokit, get, paginate } = makeOctokit({
142+
paginateResult: [
143+
{ sha: "old1", commit: { message: "m1" } },
144+
{ sha: "old2", commit: { message: "m2" } },
145+
{ sha: "shaHead", commit: { message: "tip msg" } },
146+
],
147+
});
148+
149+
const result = await fetchPR(
150+
"pytorch",
151+
"pytorch",
152+
"123",
153+
octokit,
154+
"shaHead"
155+
);
156+
157+
expect(paginate).toHaveBeenCalledTimes(1);
158+
expect(get).not.toHaveBeenCalled();
159+
expect(result.title).toBe("CH title");
160+
expect(result.body).toBe("CH body");
161+
expect(result.shas).toEqual([
162+
{ sha: "old1", title: "m1" },
163+
{ sha: "old2", title: "m2" },
164+
{ sha: "shaHead", title: "tip msg" },
165+
]);
166+
});
167+
});

0 commit comments

Comments
 (0)