perf: reduce GitHub API calls for GHES deployments#2298
Draft
ebellefontaine wants to merge 1 commit intoqodo-ai:mainfrom
Draft
perf: reduce GitHub API calls for GHES deployments#2298ebellefontaine wants to merge 1 commit intoqodo-ai:mainfrom
ebellefontaine wants to merge 1 commit intoqodo-ai:mainfrom
Conversation
Cache commits list and language stats to avoid duplicate fetches (Phases 1-2). Behind new `github.minimize_api_calls` flag (default off): - Skip file content fetches for /review when patches exist (Phase 4) - Suppress temporary "Preparing review..." comment (Phase 5) - Use PR object labels instead of extra GET call (Phase 6) Reduces API calls from 14+2N to 9 (constant) for /review. No impact on /improve or /describe — they always fetch full content.
Contributor
Review Summary by QodoReduce GitHub API calls for GHES deployments via caching and conditional skipping
WalkthroughsDescription• Cache commits list and language stats to eliminate duplicate API fetches • Skip file content fetches for /review when patches exist (behind flag) • Suppress temporary "Preparing review..." comment when minimizing API calls • Use PR object labels instead of extra GET call for deduplication • Add comprehensive unit tests covering all optimization phases Diagramflowchart LR
A["PR Review Request"] --> B["Check minimize_api_calls flag"]
B -->|False| C["Standard flow: fetch all content"]
B -->|True| D["Optimized flow"]
D --> E["Use cached commits"]
D --> F["Use cached languages"]
D --> G["Skip file content fetches"]
D --> H["Skip temp comment"]
D --> I["Use PR object labels"]
C --> J["14 + 2N API calls"]
E --> K["9 constant API calls"]
F --> K
G --> K
H --> K
I --> K
File Changes1. pr_agent/git_providers/github_provider.py
|
Contributor
Code Review by Qodo
1. Unused pytest import
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
|
||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest |
Contributor
There was a problem hiding this comment.
1. Unused pytest import 📘 Rule violation ⚙ Maintainability
pytest is imported but never used in the new unit test file, which is expected to fail Ruff/CI lint checks. This introduces a tooling compliance issue in newly added code.
Agent Prompt
## Issue description
`pytest` is imported but not used, which should fail Ruff (unused import).
## Issue Context
This import was introduced in the new test module `tests/unittest/test_minimize_api_calls.py`.
## Fix Focus Areas
- tests/unittest/test_minimize_api_calls.py[15-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+273
to
+276
| get_logger().info( | ||
| f"Filtered out [ignore] files for pull request:", | ||
| extra={"files": names_original, "filtered_files": names_new}, | ||
| ) |
Contributor
There was a problem hiding this comment.
2. Pointless f-string in log 📘 Rule violation ⚙ Maintainability
A log message is written as an f-string without any interpolated values, which is a Ruff violation (F541) and can fail linting. This was introduced/modified in the PR and should be converted to a normal string.
Agent Prompt
## Issue description
A logging call uses an f-string with no `{}` placeholders, which triggers Ruff `F541`.
## Issue Context
This appears in the `get_diff_files()` logging path where an `extra=` dict is provided.
## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[273-276]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+299
to
306
| # When minimize_api_calls is enabled and not in incremental mode, | ||
| # skip file content fetches for files that already have a patch from | ||
| # the PR files endpoint. This eliminates up to 2N API calls (N = files). | ||
| minimize = get_settings().get("github.minimize_api_calls", False) | ||
| skip_content = minimize and not self.incremental.is_incremental | ||
|
|
||
| counter_valid = 0 | ||
| for file in files: |
Contributor
There was a problem hiding this comment.
3. Minimize breaks non-review tools 🐞 Bug ≡ Correctness
GithubProvider.get_diff_files() now skips fetching base/head file content whenever github.minimize_api_calls is enabled and a patch exists, regardless of which tool is running. Tools like /improve (PRCodeSuggestions) and /add_docs rely on head_file/base_file for indentation and deduplication, so with the flag enabled they can produce mis-indented suggestions and bypass duplicate-detection guards.
Agent Prompt
### Issue description
`github.minimize_api_calls` currently changes `GithubProvider.get_diff_files()` behavior globally (for all tools), causing `FilePatchInfo.head_file`/`base_file` to be empty for many files. This breaks or degrades tools that require full file content (e.g., `/improve` dedenting + duplication guards, `/add_docs` indentation/placement).
### Issue Context
The optimization should only apply to `/review` (where `get_pr_diff(..., disable_extra_lines=True)` is used), but `get_diff_files()` is also used by other tools that pass `disable_extra_lines=False` and expect populated file contents.
### Fix Focus Areas
- pr_agent/git_providers/github_provider.py[299-353]
- pr_agent/tools/pr_reviewer.py[165-222]
- pr_agent/tools/pr_code_suggestions.py[367-373]
### What to change (one acceptable approach)
- Remove the global `skip_content = minimize ...` behavior from `GithubProvider.get_diff_files()`.
- Instead, make skipping content an explicit, review-only mode, e.g.:
- Add a provider attribute (e.g., `self._skip_content_fetch = False`) and only set it to `True` inside `PRReviewer` when running `/review`.
- Or set a request-scoped flag in `starlette_context.context` in `PRReviewer` before calling `get_pr_diff()`, and have `get_diff_files()` consult *that* (not the global config) to decide.
- Ensure tools that need content (`/improve`, `/add_docs`, etc.) never run with content fetching disabled even if the global config flag is true.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Author
|
Moving to draft until I resolve the above issues reported |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When running pr-agent against a self-hosted GitHub Enterprise Server (GHES) appliance, every API call translates directly into HTTP handler work on the appliance's CPU and memory. A single
/reviewinvocation currently makes 14 + 2N API calls (where N is the number of changed files, capped at 50). For a 20-file PR that's 54 calls; for a 50-file PR it's 114. At scale across a team, this adds meaningful load to the appliance -- especially during busy review periods.GHES rate limiting is typically disabled by default, so the problem doesn't surface as 429 errors. It shows up as degraded appliance performance under concurrent pr-agent activity.
Approach
Two categories of changes:
Unconditional caching (always active):
__init__soget_commit_messages()reuses it instead of fetching againget_languages()results so repeated calls within a review cycle don't re-fetchConditional optimizations (behind
github.minimize_api_calls = true, default off):/reviewwhen the PR files endpoint already provides the patch. This is the big win -- eliminates up to 2N callsThe conditional changes only affect
/review. The/improveand/describetools always fetch full file content because they need it for indentation analysis and deduplication guards.Results
The remaining 9 calls are the irreducible minimum: repo, PR, commits, config, languages, files, compare, labels (conditional), and the review comment itself.
What's guarded and why
handle_patch_deletions()usesedit_type == DELETEDinstead of inferring deletion from empty content when the flag is on, since empty content now means "intentionally skipped" rather than "file was deleted"load_large_diff)disable_extra_linesis passed through toget_pr_diff()when minimizing, using the existing parameter rather than mutating the settings singletonConfig
Test plan
/reviewon a test PR with flag on/off and compare output/improveand/describewith flag on to confirm they still fetch content