damccorm commented on code in PR #39980: URL: https://github.com/apache/beam/pull/39980#discussion_r3925040197
########## scripts/ci/pr-bot/shared/geminiReviewerAdvisor.ts: ########## @@ -0,0 +1,426 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { + PrHistoryContext, + CandidateContributor, + TouchedFileContext, +} from "./gitHistory"; + +/** + * Interface representing an individual recommended reviewer. + */ +export interface ReviewerRecommendation { + readonly username: string; + readonly role: "primary" | "secondary"; + readonly isCommitter: boolean; + readonly expertise: string; + readonly coveredFiles: readonly string[]; +} + +/** + * Interface representing an alternate reviewer suggestion. + */ +export interface AlternateReviewer { + readonly username: string; + readonly expertise: string; +} + +/** + * Result structure produced by the reviewer advisor. + */ +export interface ReviewerAdviceResult { + readonly selectedReviewers: readonly ReviewerRecommendation[]; + readonly alternateReviewers: readonly AlternateReviewer[]; + readonly reasoning: string; + readonly source: "gemini" | "heuristic-fallback"; +} + +/** + * Interface for LLM clients that can generate structured JSON. + */ +export interface IGeminiClient { + generateJson<T>(prompt: string): Promise<T>; +} + +/** + * Standard HTTP Gemini client using global fetch. + */ +export class GeminiClient implements IGeminiClient { + private readonly apiKey: string; + private readonly model: string; + + constructor(apiKey: string, model: string = "gemini-2.5-flash") { + this.apiKey = apiKey; + this.model = model; + } + + async generateJson<T>(prompt: string): Promise<T> { + if (!this.apiKey) { + throw new Error("GEMINI_API_KEY is not configured."); + } + + const url = `https://generativelanguage.googleapis.com/v1beta/models/${encodeURIComponent( + this.model + )}:generateContent?key=${encodeURIComponent(this.apiKey)}`; + + const response = await fetch(url, { + method: "POST", + headers: { + "Content-Type": "application/json", + }, + body: JSON.stringify({ + contents: [ + { + role: "user", + parts: [{ text: prompt }], + }, + ], + generationConfig: { + temperature: 0.1, + responseMimeType: "application/json", + }, + }), + }); + + if (!response.ok) { + const errorText = await response.text(); + throw new Error( + `Gemini API request failed with status ${response.status}: ${errorText}` + ); + } + + const data: any = await response.json(); + const candidateText = data?.candidates?.[0]?.content?.parts?.[0]?.text; + + if (!candidateText) { + throw new Error("Empty or invalid candidate response from Gemini API."); + } + + return JSON.parse(candidateText) as T; + } +} + +/** + * Configuration options for the Gemini Reviewer Advisor. + */ +export interface ReviewerAdvisorOptions { + readonly geminiClient?: IGeminiClient; + readonly committerCheck?: (username: string) => Promise<boolean>; + readonly exclusionList?: readonly string[]; + readonly maxReviewers?: number; +} + +/** + * Advisor that analyzes PR git history and selects optimal reviewers using Gemini or heuristic fallback. + */ +export class GeminiReviewerAdvisor { + private readonly client?: IGeminiClient; + private readonly committerCheck: (username: string) => Promise<boolean>; + private readonly exclusionList: readonly string[]; + private readonly maxReviewers: number; + + constructor(options: ReviewerAdvisorOptions = {}) { + this.client = options.geminiClient; + this.committerCheck = options.committerCheck ?? (async () => false); + this.exclusionList = options.exclusionList ?? []; + this.maxReviewers = options.maxReviewers ?? 2; + } + + /** + * Constructs the prompt instructing Gemini on how to select reviewers. + * + * @param context Extracted git and PR history. + * @param committers Map of username to committer status. + * @returns Detailed prompt string. + */ + public buildPrompt( + context: PrHistoryContext, + committers: Readonly<Record<string, boolean>> + ): string { + const fileSummaries = context.touchedFiles.map((file) => { + const commitSummaries = file.recentCommits + .slice(0, 5) + .map( + (c) => + ` - [${c.date}] ${c.authorLogin || c.authorName}: ${c.subject}` + ) + .join("\n"); + + return `- File: ${file.path} (+${file.additions}, -${ + file.deletions + }, changes: ${file.changes}${ + file.isNewFile ? " [NEW FILE]" : "" + })\n Recent Commits:\n${commitSummaries || " (No recent commits)"}`; + }); + + const candidateSummaries = context.candidates.map((c) => { + const isCommitter = committers[c.login] ?? false; + return `- @${c.login} (${c.name}): ${ + c.commitCount + } commits, last active ${ + c.lastCommitDate + }, committer=${isCommitter}. Files touched: ${c.touchedFilePaths.join( + ", " + )}`; + }); + + const exclusions = + this.exclusionList.map((e) => `@${e}`).join(", ") || "(none)"; + + return `You are the Apache Beam Code Review Assigner. +Your goal is to choose a small, optimal set of expert reviewers for a pull request based on real git history and file churn. Review Comment: > The gemini infra is going to be painful since it will need to be a secret we regularly rotate. > We could ditch gemini and just go with the heuristic score, for example. Ok, actually I thought a bit more about this and I don't think this will be an issue - we just shouldn't use a GEMINI_API_KEY, we can just use Vertex + gcloud auth. We do this in our model handler if no API key is provided, and our ITs rely on this - https://github.com/apache/beam/blob/562273c38b0c67071d809b338fc0c45658f39e51/sdks/python/apache_beam/ml/inference/gemini_inference.py#L193 The idea has grown on me, I think my primary concern is more around the context and how we're prompting the model. > I'm biased because I get on there for "java" and "website" and "fallback" and realistically there's always a better choice. I don't think load balancing to people who are that vaguely related is necessarily good. Could you give examples of 1 or 2 prs as case studies of where we could do better and what would have led to a better outcome? I think the most important part of this is that we need to pick the right set of things to prioritize when choosing a reviewer. The criteria I can think of are: 1. Competence: Don't assign a reviewer who will be incapable of providing a good review. 2. Fairness: Don't assign a reviewer to too many PRs. 3. Opt-out: Allow reviewers to opt out of being assigned too many PRs (or limit the volume). 4. Affinity: Try to assign reviewers who are most active in an area. 5. Inclusion: Try to include/train reviewers who may not be the best option, but are contributing more. I tried to order those criteria from most to least important. I think right now, the prompt fully addresses (4) and partially addresses (1)/(5). It doesn't try to address (2) or (3). I think the current system generally accomplishes (1) and (3). It tries to accomplish (2), (4), and (5), but probably doesn't do a great job with any of them. Fairness is the one that I think is hardest. For reference, if you look at the last calendar year and filter out both reviews done on the reviewer's own PR and dependabot reviews (usually 1-liners): - [Yi](https://github.com/apache/beam/pulls?q=is%3Apr+created%3A%3E%3D2025-09-04+reviewed-by%3Aabacn+-author%3Aabacn+-label%3Adependencies) has reviewed the most PRs with 501 - [I](https://github.com/apache/beam/pulls?q=is%3Apr+created%3A%3E%3D2025-09-04+reviewed-by%3Adamccorm+-author%3Adamccorm+-label%3Adependencies) am second with 397 - For comparison, [you](https://github.com/apache/beam/pulls?q=is%3Apr+created%3A%3E%3D2025-09-04+reviewed-by%3Akennknowles+-author%3Akennknowles+-label%3Adependencies+) are at 105. I did a check of other common reviewers and couldn't find anyone with more than 129 reviews For both Yi and me, this is driven by a lot of manual assignment, so I'm not sure if the system can totally fix it. But I worry that an affinity based assigner will overassign Yi because he is very active across the codebase (and is likely the best reviewer by heuristics) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
