Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add experimental AI PR review #187

Merged
merged 3 commits into from
Nov 22, 2023
Merged

feat: Add experimental AI PR review #187

merged 3 commits into from
Nov 22, 2023

Conversation

scott-codecov
Copy link
Contributor

@scott-codecov scott-codecov commented Nov 20, 2023

Enable the feature in your codecov.yml and you'll start receiving GPT-4 powered PR reviews. The YAML is checked in the PullSyncTask and, if that feature is enabled, then it spins off a new AiPrReviewTask which handles the interactions with GitHub and OpenAI. This is GitHub only right now since the PR review API is a bit specific to GitHub (it's commenting on specific lines on code).

The GPT-4 prompt could probably use a lot of work as I have very little experience with this. It's currently only passing in the Git patch and the comments received back from the OpenAI API have been somewhat lackluster. Providing additional context about the codebase could potentially help here but is out-of-scope for this initial POC.

Depends on codecov/shared#87 for the YAML schema update

TODO: update shared SHA once the PR linked above has been merged to main

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Merging #187 (3de8687) into main (64da979) will decrease coverage by 0.04%.
The diff coverage is 95.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
- Coverage   98.34%   98.30%   -0.04%     
==========================================
  Files         376      380       +4     
  Lines       28088    28366     +278     
==========================================
+ Hits        27622    27886     +264     
- Misses        466      480      +14     
Flag Coverage Δ
integration 98.34% <95.00%> (-0.04%) ⬇️
latest-uploader-overall 98.34% <95.00%> (-0.04%) ⬇️
unit 98.34% <95.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.67% <93.06%> (-0.07%) ⬇️
OutsideTasks 98.13% <96.53%> (-0.02%) ⬇️
Files Coverage Δ
conftest.py 94.77% <100.00%> (+0.07%) ⬆️
services/tests/test_ai_pr_review.py 100.00% <100.00%> (ø)
tasks/__init__.py 100.00% <100.00%> (ø)
tasks/tests/integration/test_ai_pr_review.py 100.00% <100.00%> (ø)
tasks/sync_pull.py 97.95% <50.00%> (-0.67%) ⬇️
tasks/ai_pr_review.py 83.33% <83.33%> (ø)
services/ai_pr_review.py 95.26% <95.26%> (ø)

This change has been scanned for critical changes. Learn more

@codecov-staging
Copy link

codecov-staging bot commented Nov 20, 2023

Codecov Report

Merging #187 (3de8687) into main (64da979) will decrease coverage by 0.04%.
The diff coverage is 95.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
- Coverage   98.37%   98.34%   -0.04%     
==========================================
  Files         347      351       +4     
  Lines       27443    27721     +278     
==========================================
+ Hits        26998    27262     +264     
- Misses        445      459      +14     
Flag Coverage Δ
integration 98.34% <95.00%> (-0.04%) ⬇️
latest-uploader-overall 98.34% <95.00%> (-0.04%) ⬇️
unit 98.34% <95.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.79% <93.06%> (-0.08%) ⬇️
OutsideTasks 98.13% <96.53%> (-0.02%) ⬇️
Files Coverage Δ
conftest.py 94.77% <100.00%> (+0.07%) ⬆️
services/tests/test_ai_pr_review.py 100.00% <100.00%> (ø)
tasks/__init__.py 100.00% <100.00%> (ø)
tasks/tests/integration/test_ai_pr_review.py 100.00% <100.00%> (ø)
tasks/sync_pull.py 97.94% <50.00%> (-0.67%) ⬇️
tasks/ai_pr_review.py 83.33% <83.33%> (ø)
services/ai_pr_review.py 95.26% <95.26%> (ø)

@codecov-qa
Copy link

codecov-qa bot commented Nov 20, 2023

Codecov Report

Merging #187 (3de8687) into main (64da979) will decrease coverage by 0.04%.
The diff coverage is 95.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
- Coverage   98.37%   98.34%   -0.04%     
==========================================
  Files         347      351       +4     
  Lines       27443    27721     +278     
==========================================
+ Hits        26998    27262     +264     
- Misses        445      459      +14     
Flag Coverage Δ
integration 98.34% <95.00%> (-0.04%) ⬇️
latest-uploader-overall 98.34% <95.00%> (-0.04%) ⬇️
unit 98.34% <95.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.79% <93.06%> (-0.08%) ⬇️
OutsideTasks 98.13% <96.53%> (-0.02%) ⬇️
Files Coverage Δ
conftest.py 94.77% <100.00%> (+0.07%) ⬆️
services/tests/test_ai_pr_review.py 100.00% <100.00%> (ø)
tasks/__init__.py 100.00% <100.00%> (ø)
tasks/tests/integration/test_ai_pr_review.py 100.00% <100.00%> (ø)
tasks/sync_pull.py 97.94% <50.00%> (-0.67%) ⬇️
tasks/ai_pr_review.py 83.33% <83.33%> (ø)
services/ai_pr_review.py 95.26% <95.26%> (ø)

Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid. Interesting project, excited to see what comes out of it.


async def fetch_diff(self) -> str:
async with self.torngit.get_client() as client:
diff = await self.torngit.api(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why you decided to implement this here instead of in torngit.
It's the same request from get_pull_request (but I think you are using more of the data)

I understand that this feature is experimental so I think it's better to keep it as self-contained as possible. I'm not asking you to change anything, just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference here is the Accept header below. By passing that header, GitHub will return the raw Git patch (which you can also get while logged in by appending .diff to the end of a normal PR URL).


try:
data = json.loads(res)
except json.decoder.JSONDecodeError:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have metrics for these errors so we can track how successful this thing is (in terms of delivering any response, obviously it wouldn't measure how effective the response is)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 3de8687

@scott-codecov
Copy link
Contributor Author

scott-codecov commented Nov 22, 2023

Thanks for the review @giovanni-guidini! Would you mind taking a quick look at codecov/shared#87 as well so I can get the shared version updated here?

EDIT: nevermind - @joseph-sentry did it - thanks!

@scott-codecov scott-codecov merged commit e187e61 into main Nov 22, 2023
13 of 26 checks passed
@scott-codecov scott-codecov deleted the scott/ai-pr-review branch November 22, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants