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

Add Score Review Feature #89

Merged
merged 7 commits into from
Jul 19, 2023
Merged

Add Score Review Feature #89

merged 7 commits into from
Jul 19, 2023

Conversation

zmeir
Copy link
Contributor

@zmeir zmeir commented Jul 18, 2023

Type of PR:
Enhancement


PR Description:
This PR introduces a new feature to the PR Reviewer tool, allowing it to calculate a score for each PR. The score is a float value between 0 and 100, representing the quality of the PR. The PR also includes updates to the configuration files to enable or disable this feature.


PR Main Files Walkthrough:
-pr_agent/algo/utils.py: Added a new emoji for the score in the markdown conversion function.
-pr_agent/tools/pr_reviewer.py: Added a new variable 'require_score' to the PR Reviewer's variables, which is fetched from the settings.
-CONFIGURATION.md: Updated the documentation to include the new 'require_score_review' flag.
-pr_agent/settings/configuration.toml: Added a new flag 'require_score_review' to enable or disable the score review feature.
-pr_agent/settings/pr_reviewer_prompts.toml: Updated the prompts file to include a new 'Score' field in the JSON schema if the 'require_score' flag is set to true.

This can help teams compare the review of the PR agent with that of a human reviewer, and fine-tune a score threshold for automatic approval where they decide the agent's review is satisfactory.
@zmeir
Copy link
Contributor Author

zmeir commented Jul 18, 2023

/describe

@zmeir
Copy link
Contributor Author

zmeir commented Jul 18, 2023

/review

@github-actions github-actions bot changed the title Add configuration to request a score for the PR Add Score Review Feature Jul 18, 2023
@github-actions
Copy link
Contributor

PR Analysis

  • 🎯 Main theme: Adding a score configuration to the PR review process
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • Insights from user's answer: No user answers were provided
  • Focused PR: Yes, the PR is focused as it introduces a new feature of adding a score to the PR review process and all changes are related to this feature.
  • 🔒 Security concerns: No, the PR does not introduce any obvious security concerns as it mainly deals with configuration settings and does not involve any sensitive data handling or risky operations.

PR Feedback

  • 💡 General PR suggestions: The PR is well structured and the changes are consistent with the PR's title and description. However, it would be beneficial to add tests to verify the new functionality. Also, consider adding error handling for the score input to ensure it's within the valid range (0-100).

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review - Request a review of the latest update to the PR.
/describe - Modify the PR title and description based on the contents of the PR.
/improve - Suggest improvements to the code in the PR. These will be provided as pull request comments, ready to commit.
/ask - Pose a question about the PR.

pr_agent/tools/pr_reviewer.py Show resolved Hide resolved
pr_agent/settings/configuration.toml Outdated Show resolved Hide resolved
pr_agent/settings/pr_reviewer_prompts.toml Show resolved Hide resolved
@mrT23
Copy link
Collaborator

mrT23 commented Jul 19, 2023

PR Analysis

  • 🎯 Main theme: The PR introduces a new feature to the PR Reviewer tool, allowing it to calculate a score for each PR. The score is a float value between 0 and 100, representing the quality of the PR. The PR also includes updates to the configuration files to enable or disable this feature.
  • 📌 Type of PR: Enhancement
  • 🏅 Score: 85, the PR is well-structured and the changes are clearly explained. However, it lacks tests for the new feature, which could potentially lead to bugs or issues in the future.
  • 🧪 Relevant tests added: No
  • Insights from user's answer: The user did not provide any additional insights.
  • Focused PR: Yes, the PR has a clear and coherent title and description, and all PR code diff changes are properly derived from the title and description.
  • 🔒 Security concerns: No, the changes made in this PR do not appear to introduce any potential security concerns.

PR Feedback

  • 💡 General PR suggestions: The PR is generally well-structured and the changes are clearly explained. However, it would be beneficial to include tests for the new feature to ensure its functionality and prevent potential issues in the future. Additionally, it would be helpful to provide more detailed information in the PR description about how the score is calculated.

Comment on lines 25 to 26
"type": "float",
"description": "Rate this PR on a scale of 0-100 (inclusive), where 0 means the worst possible code, and 100 means code of the highest quality without any bugs or performace issues that is ready to be merged immediately and run in production at scale."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"type": "float",
"description": "Rate this PR on a scale of 0-100 (inclusive), where 0 means the worst possible code, and 100 means code of the highest quality without any bugs or performace issues that is ready to be merged immediately and run in production at scale."
"type": "str",
"description": "Rate this PR on a scale of 0-100 (inclusive), where 0 means the worst possible PR code, and 100 means PR code of the highest quality, without any bugs or performance issues, that is ready to be merged immediately and run in production at scale. Explain shortly the reason for the score you gave."

@mrT23
Copy link
Collaborator

mrT23 commented Jul 19, 2023

Hi @zmeir. That's a nice idea for an additional sub-tool.

  • In order to get it to work consistently, you must also add it as an example output (see below). Without it I got no score output.

  • Also, the prompt should be a bit tightened, I added a code comment. I think its important to explain the score, otherwise, it can seem a bit arbitrary

  • Other than that, my main dilemma is if to enable it by default, or not. I like that it gives direct measurable feedback. However, people can be sensitive. Don't you think some may find this "semi-offensive" as a default option ?


Example output:
'
{
    "PR Analysis":
    {
        "Main theme": "xxx",
        "Type of PR": "Bug fix",
{%- if require_tests %}
        "Relevant tests added": "No",
{%- endif %}
{%- if require_score %}
        "Score": "95 - because...",
{%- endif %}

p.s.
this is your score :-)
image

@zmeir
Copy link
Contributor Author

zmeir commented Jul 19, 2023

Hi @mrT23, thanks for the review. To answer your comments:

That's a nice idea for an additional sub-tool.

I was thinking since it's a small output and very similar to the theme of the other sections in the review analysis, that it should belong there. My thought was that separating it to a sub-tool is similar to separating the "Security concerns" or "Relevant tests added" parts of the PR Analysis, which felt wrong.

In order to get it to work consistently, you must also add it as an example output (see below). Without it I got no score output.

Strange, it actually worked for me with the code as I submitted it. For example:
image
Maybe it has to do with the different type in the prompt? (I used "float" but I see in your suggestion you put "str")
I'll try running it again on this PR to show what it yields.

Also, the prompt should be a bit tightened, I added a code comment. I think its important to explain the score, otherwise, it can seem a bit arbitrary

I see what you mean, I just thought that this part should be very concise and only add a score that we can then use as a sort of reliability threshold for automatic approval. I thought it should behave in a similar way to the "Type of PR" or "Relevant tests added" fields. I like the idea that the "PR Analysis" part is a sort of "checklist" of simple measurements that you can get a feel of the quality of this PR at a glance, and if you want more details you look at the "PR Feedback" section.
Also, don't you think the explanation to the score is very similar to "General PR suggestions" (looking at your example). Isn't it a bit redundant then? I'm assuming we don't want to tire the users with repeated information/suggestions.

Other than that, my main dilemma is if to enable it by default, or not. I like that it gives direct measurable feedback. However, people can be sensitive. Don't you think some may find this "semi-offensive" as a default option ?

Agree completely. I will set the default to false.

@zmeir
Copy link
Contributor Author

zmeir commented Jul 19, 2023

PR Analysis

  • 🎯 Main theme: Add Score Review Feature
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • Insights from user's answer: No insights provided by the user.
  • Focused PR: Yes, the PR has a clear and coherent title and description, and all PR code diff changes are properly derived from the title and description.
  • 🔒 Security concerns: No, this PR does not introduce possible security concerns or issues.

PR Feedback

  • 💡 General PR suggestions: The PR introduces a new feature to calculate a score for each PR, which is a good enhancement. However, it would be better if there were tests accompanying this new feature to ensure its functionality and correctness. Also, it would be helpful to provide more details on how the score is calculated and what factors are considered in the calculation.

pr_agent/tools/pr_reviewer.py Show resolved Hide resolved
pr_agent/settings/pr_reviewer_prompts.toml Show resolved Hide resolved
pr_agent/algo/utils.py Show resolved Hide resolved
@zmeir
Copy link
Contributor Author

zmeir commented Jul 19, 2023

PR Analysis

  • 🎯 Main theme: Add Score Review Feature
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • Insights from user's answer: No insights provided by the user.
  • Focused PR: Yes, the PR has a clear and coherent title and description, and all PR code diff changes are properly derived from the title and description.
  • 🔒 Security concerns: No, this PR does not introduce possible security concerns or issues.

PR Feedback

  • 💡 General PR suggestions: The PR introduces a new feature to calculate a score for each PR, which is a good enhancement. However, it would be better if there were tests accompanying this new feature to ensure its functionality and correctness. Also, it would be helpful to provide more details on how the score is calculated and what factors are considered in the calculation.

@mrT23 I see what you mean, it really didn't output the Score when using my branch. When I tested it locally before opening the PR it did work, I'll check and see if there's something I missed...

@zmeir
Copy link
Contributor Author

zmeir commented Jul 19, 2023

@mrT23 this is very strange - when I request review on this PR I don't get a score, but when I try it on a few of my PRs in another repo (private) it's working:
image

Anyway, I added there relevant example output like you suggested, and now it works for all the PRs I've tested :)
image
image

@mrT23
Copy link
Collaborator

mrT23 commented Jul 19, 2023

@zmeir Ok, if the default is off, maybe we can keep the output short, just as a number.
so just change 'float' to 'int', and fix the typos in the prompt. i don't think the score should be 91.53 :-)

Regarding example output, it makes outputting a json more stable, especially for complicated json like ours

@zmeir
Copy link
Contributor Author

zmeir commented Jul 19, 2023

@mrT23 please see the latest changes and let me know what you think

@zmeir Ok, if the default is off, maybe we can keep the output short, just as a number. so just change 'float' to 'int', and fix the typos in the prompt. i don't think the score should be 91.53 :-)

Done. Although I'm usually a very precise person so I do appreciate the decimal precision, but I'm probably the odd one out so I see your point :)

Regarding example output, it makes outputting a json more stable, especially for complicated json like ours

Good to know, I'm very new to working with OpenAI and LLMs in general (relatively speaking, aren't we all?), so I'm always happy to learn new stuff :)

@zmeir
Copy link
Contributor Author

zmeir commented Jul 19, 2023

Example for a truly horrible PR just to make sure the score isn't always high:
zmeir#1

image

@mrT23 mrT23 merged commit 577f24d into Codium-ai:main Jul 19, 2023
@Codium-ai Codium-ai deleted a comment from hussam789 Jul 19, 2023
@Codium-ai Codium-ai deleted a comment from hussam789 Jul 19, 2023
@okotek
Copy link
Contributor

okotek commented Jul 19, 2023

/review -i

@github-actions
Copy link
Contributor

Incremental PR Review

  • ⏮️ Review for commits since previous PR-Agent review: Starting from commit f8aea90

PR Analysis

  • 🎯 Main theme: Introduction of a new score review feature in the PR Reviewer tool
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • Focused PR: Yes, the PR has a clear title and description that matches the code changes made in the PR.
  • 🔒 Security concerns: No, the changes made in this PR do not introduce any apparent security concerns.

PR Feedback

  • 💡 General PR suggestions: The PR is well-structured and introduces a new feature as described. However, it would be beneficial to include tests for the new feature to ensure it works as expected. Additionally, the change in the type of the 'Score' field from 'float' to 'int' in the JSON schema might limit the precision of the score. If a more precise score is desired, consider changing it back to 'float'.

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review - Request a review of the latest update to the PR.
/describe - Modify the PR title and description based on the contents of the PR.
/improve - Suggest improvements to the code in the PR. These will be provided as pull request comments, ready to commit.
/ask - Pose a question about the PR.

@okotek
Copy link
Contributor

okotek commented Jul 19, 2023

/review -i

@github-actions
Copy link
Contributor

Incremental PR Review

  • ⏮️ Review for commits since previous PR-Agent review: Starting from commit f8aea90

PR Analysis

  • 🎯 Main theme: Add Score Review Feature
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • Focused PR: Yes, the PR has a clear and coherent title and description, and all PR code diff changes are properly derived from the title and description.
  • 🔒 Security concerns: No, the changes made in this PR do not introduce any apparent security concerns.

PR Feedback

  • 💡 General PR suggestions: The PR is well-structured and the changes are well-documented. However, it would be beneficial to add tests to ensure the new feature works as expected. Additionally, the change in the 'require_score_review' flag in the 'configuration.toml' file from 'true' to 'false' is not explained in the PR description. If this is an intentional change, please include a reason for it in the PR description. If it is not intentional, please revert it.

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review Request a review of the latest update to the PR.
/review -i Incremental review - consider only the changes from the last review.
/describe Modify the PR title and description based on the contents of the PR.
/improve Suggest improvements to the code in the PR. These will be provided as pull request comments, ready to commit.
/ask Pose a question about the PR.

@zmeir zmeir deleted the zmeir-review_score branch July 23, 2023 07:12
yochail pushed a commit to yochail/pr-agent that referenced this pull request Feb 11, 2024
Copy link

@dharmesh-growexxer dharmesh-growexxer left a comment

Choose a reason for hiding this comment

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

GitHub Copilot Review

Title: feat: 🎸 move to prod changes
Description: image

Changes:

server/util/constants.js

@@ -47,11 +47,11 @@ module.exports = {
LOCAL: 'local',
DEV: 'dev',
PRODUCTION: 'production'
},

  • AWS_S3_PUBLIC_BUCKET: 'dev-use2-assests',
  • AWS_S3_PUBLIC_BUCKET: process.env.AWS_S3_PUBLIC_BUCKET,
    CLIENT_INFO: {
    EMAIL: '[email protected]'
    },
    DATE_FORMAT: 'MM/DD/YYYY',

package.json

@@ -6,9 +6,9 @@
"scripts": {
"prestart": "NODE_ENV=local node_modules/.bin/sequelize db:migrate",
"prestart:dev": "NODE_ENV=development node_modules/.bin/sequelize db:migrate",
"prestart:stg": "NODE_ENV=staging node_modules/.bin/sequelize db:migrate",

  • "prestart:prod": "NODE_ENV=production node getEnvs.js && NODE_ENV=production node_modules/.bin/sequelize db:migrate",
  • "prestart:prod": "NODE_ENV=production node_modules/.bin/sequelize db:migrate",
    "start": "NODE_ENV=local nodemon .",
    "start:dev": "NODE_ENV=development node index.js",
    "start:stg": "NODE_ENV=staging node index.js",
    "start:prod": "NODE_ENV=production node index.js",

Copy link

@dharmesh-growexxer dharmesh-growexxer left a comment

Choose a reason for hiding this comment

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

GitHub Copilot Review Guide 🤖

PR Overview

Category Details
Title feat: 🎸 move to prod changes
Description image

|

Code Analysis

Category Details
Changes Overview Analyzing 34 lines of changes
Main Focus Code quality and potential improvements

Key Findings

  1. Code Structure
  2. Best Practices
  3. Potential Improvements

Detailed Review

server/util/constants.js

@@ -47,11 +47,11 @@ module.exports = {
LOCAL: 'local',
DEV: 'dev',
PRODUCTION: 'production'
},

  • AWS_S3_PUBLIC_BUCKET: 'dev-use2-assests',
  • AWS_S3_PUBLIC_BUCKET: process.env.AWS_S3_PUBLIC_BUCKET,
    CLIENT_INFO: {
    EMAIL: '[email protected]'
    },
    DATE_FORMAT: 'MM/DD/YYYY',

package.json

@@ -6,9 +6,9 @@
"scripts": {
"prestart": "NODE_ENV=local node_modules/.bin/sequelize db:migrate",
"prestart:dev": "NODE_ENV=development node_modules/.bin/sequelize db:migrate",
"prestart:stg": "NODE_ENV=staging node_modules/.bin/sequelize db:migrate",

  • "prestart:prod": "NODE_ENV=production node getEnvs.js && NODE_ENV=production node_modules/.bin/sequelize db:migrate",
  • "prestart:prod": "NODE_ENV=production node_modules/.bin/sequelize db:migrate",
    "start": "NODE_ENV=local nodemon .",
    "start:dev": "NODE_ENV=development node index.js",
    "start:stg": "NODE_ENV=staging node index.js",
    "start:prod": "NODE_ENV=production node index.js",

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.

4 participants