[code_review] Misc improvements part 5 #5652
Open
Conversation
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
Replaced usage of PhabricatorReviewData.get_patch_by_id with direct instantiation of PhabricatorPatch in both dataset creation and evaluation notebooks.
Eliminated the abstract and concrete implementations of get_review_request_by_id from ReviewData and its subclasses, as they are no longer used.
Introduces a cached `get_phabricator_client` function to manage Phabricator API client instantiation and replaces direct usage of `phabricator.PHABRICATOR_API` with the new client throughout the module. Removes legacy secret fetching and updates environment variable handling for backward compatibility.
Relocated the imports of db and phabricator modules inside the get_all_inline_comments method to avoid unnecessary top-level imports. This change helps reduce import overhead.
The implementation is not complete. It can live outside bugbug.
Converted generate_review_comments and run methods in CodeReviewTool to async and updated internal calls to use async/await. This prepares the tool for asynchronous operations, improving scalability and responsiveness.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the code review tool infrastructure to modernize HTTP client usage, simplify the architecture by removing unused code, and introduce async/await patterns for better performance. The changes remove the Swarm platform implementation, introduce a new centralized HTTP client, and convert several methods to async.
Changes:
- Removed unused Swarm platform implementation and legacy compatibility code
- Introduced centralized HTTP client management with
get_http_client()andget_phabricator_client()functions - Converted file retrieval methods to async patterns and updated callers accordingly
- Updated scoring logic to handle missing keys gracefully when errors occur
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| notebooks/code_review_evaluation.ipynb | Updated to use PhabricatorPatch directly and made invoke method async |
| notebooks/code_review_create_dataset.ipynb | Updated to use PhabricatorPatch directly instead of PhabricatorReviewData |
| bugbug/tools/core/platforms/swarm.py | Deleted unused Swarm platform implementation |
| bugbug/tools/core/platforms/phabricator.py | Major refactoring: introduced get_phabricator_client(), converted file retrieval to async, replaced global PHABRICATOR_API usage |
| bugbug/tools/core/platforms/base.py | Made get_old_file() async and removed unused get_review_request_by_id() method |
| bugbug/tools/core/connection.py | New module providing centralized HTTP client and user agent management |
| bugbug/tools/code_review/scorer.py | Updated to handle missing keys gracefully with .get() and default values |
| bugbug/tools/code_review/langchain_tools.py | Made expand_context tool async to match new get_old_file() signature |
| bugbug/tools/code_review/agent.py | Made generate_review_comments() and run() async, removed review_data dependency |
| bugbug/tools/code_review/init.py | Removed legacy SwarmReviewData and review_data_classes exports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
suhaibmujahid
force-pushed
the
improve-revew-helper-5
branch
from
db4fea3 to
4e007a9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
suhaibmujahid
force-pushed
the
improve-revew-helper-5
branch
from
4e007a9 to
6b7fbc6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This is needed since base_commit_hash() needs to be async, and it is not worth changing this to support that here.
Converted file retrieval methods in PhabricatorPatch and related interfaces to async, replacing synchronous HTTP calls with httpx.AsyncClient.
suhaibmujahid
force-pushed
the
improve-revew-helper-5
branch
from
6b7fbc6 to
bf76361
Compare
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.
These improvements could be reviewed commit by commit.