kennknowles commented on code in PR #39980: URL: https://github.com/apache/beam/pull/39980#discussion_r3925419079
########## 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: Here's unedited agent response. I asked it to select 3 PR with at least 3 "R: " bot assignments due to slow review. My take is that... it isn't that good :-). FWIW I'm deliberately taking the blameless premise that if someone didn't respond to the "R: " assignment then they had a good reason (too busy, not the right reviewer, the PR was not ready, etc). There could, of course, be other reasons that are less blameless, but I don't think it is productive. In any case being strictly pragmatic makes sense, in the sense of "if they didn't do it, they didn't do it and we change the system not the people". ---- Here are 3 real Apache Beam pull requests that experienced repeated reviewer re-assignments under the legacy bot without receiving substantive reviews, along with the results of running our new review assigner on each of them. ────── ### PR 1: #39156 https://github.com/apache/beam/pull/39156 — [IcebergIO] Use GroupByKey for bounded inputs to avoid OOM #### What happened with the legacy bot: Because the PR touched Java code, the legacy bot attached label java and assigned reviewers via generic round-robin. It was re-assigned 6 times over 2 months: 1. 2026-06-29: R: @ahmedabu98 for label java 2. 2026-07-09: R: @chamikaramj for label java (slow review reassignment) 3. 2026-07-21: R: @ahmedabu98 for label java (slow review reassignment) 4. 2026-07-31: R: @chamikaramj for label java (slow review reassignment) 5. 2026-08-12: R: @Abacn for label java (slow review reassignment) 6. 2026-08-26: R: @ahmedabu98 for label java (slow review reassignment) #### Running the new tool: • File touched: sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/WriteToPartitions.java • Git history discovered: • [2026-07-09] Claire McGinty: (IcebergIO) bugfix: propagate config properties to RecordWriter (#39250) • [2026-05-04] Ahmed Abualsaud: [IcebergIO] Support hash distribution mode when writing rows (#38061) ### 🧭 Reviewer Assignment - R: @[email protected] (**Primary Reviewer**) *Expertise:* Active contributor to IcebergIO WriteToPartitions; recently implemented config property propagation for RecordWriter. *Selected a minimal reviewer set to keep review focused. Backup expert(s): @ahmedabu98* Note: If you would like to opt out of this review, comment `assign to next reviewer`. • Why this is better: Instead of cycling through general Java reviewers who don't work on Iceberg, it pinpoints Claire McGinty (who actively maintains the partition writing logic in this exact file) with Ahmed as backup. ────── ### PR 2: #37535 https://github.com/apache/beam/pull/37535 — Fix temp table cleanup for continuous read in BigQueryIO #### What happened with the legacy bot: This PR bounced through 12 consecutive bot re-assignments across 6 months without an in-depth review: • Assigned @Abacn → @chamikaramj → @kennknowles → @chamikaramj → @ahmedabu98 → @Abacn → @ahmedabu98 → @chamikaramj → @ahmedabu98 → @chamikaramj → @kennknowles → @ahmedabu98. • The bot blindly cycled through committers configured under the general java label. #### Running the new tool: • Files touched: • sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java • sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryStorageStreamSource.java • sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/CleanupTempTableDoFnTest.java • Git history discovered: • Automatically filtered out the PR author (@stankiewicz / Radosław Stankiewicz). • Evaluated contributors touching BigQuery storage read streams: claudevdm (lakehouse storage reads), Yi Hu (stream splitting & error handling), and reuvenlax. ### 🧭 Reviewer Assignment - R: @claudevdm (**Primary Reviewer**) *Expertise:* Contributor to BigQueryIO storage read streams and table schema handling. *Selected a minimal reviewer set to keep review focused. Backup expert(s): @[email protected], @[email protected]* Note: If you would like to opt out of this review, comment `assign to next reviewer`. • Why this is better: The PR specifically modifies continuous reading and temp table management in the BigQuery Storage API. Rather than looping through the 20+ generic Java reviewers, it identifies reviewers with direct commit history in BigQueryStorageStreamSource and BigQuery storage reads. ────── ### PR 3: #39363 https://github.com/apache/beam/pull/39363 — Fix artifact staging filenames on Windows #### What happened with the legacy bot: Because the files were under runners/java-fn-execution/, no path pattern in .github/autolabeler.yml matched. The bot fell back to assigning a fallback committer: • It assigned @chamikaramj 7 times in a row: R: @chamikaramj added as fallback since no labels match configuration • It then assigned @damccorm on the 8th attempt as fallback. #### Running the new tool: • Files touched: • runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/artifact/ArtifactStagingService.java • runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/artifact/ArtifactStagingServiceTest.java • Git history discovered: • [2026-09-01] Elia Liu: Fail fast in artifact staging when storing an artifact fails (#39367) (modified both the service and the test). • Mechanical changes (ErrorProne fixes by Radosław, Spotless bumps by Derrick) were deprioritized. ### 🧭 Reviewer Assignment - R: @[email protected] (**Primary Reviewer**) *Expertise:* Author of artifact staging error handling and test suite in ArtifactStagingServiceTest. *Selected a minimal reviewer set to keep review focused. Backup expert(s): @[email protected]* Note: If you would like to opt out of this review, comment `assign to next reviewer`. • Why this is better: The legacy bot completely failed here because no label was configured for runners/java-fn-execution, repeatedly burdening a fallback committer. The new assigner looked directly at ArtifactStagingService.java's history and immediately selected Elia Liu, who had just touched the exact staging logic and test file. ────── ### Summary of Improvements Demonstrated PR | Issue with Old Bot | Old Bot Behavior | New Tool Assignment ---------------------------|--------------------------|--------------------------------------------------|------------------------------------------------------------ #39156 (IcebergIO) | Label too broad (java) | Bounced 6 times between 3 general Java reviewers | @clairem (authored Iceberg partition writer) #37535 (BigQueryIO) | Label too broad (java) | Bounced 12 times in a 6-month loop | @claudevdm / @yathu (BigQuery storage stream contributors) #39363 (Artifact Staging) | Missing directory label | Assigned fallback reviewer 8 times in a row | @Elialiu (authored staging error handling in that file) -- 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]
