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.

Sign up for GitHub

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

Jump to bottom

refactor and update prompt #124

Merged
merged 3 commits into from
May 9, 2024
Merged

refactor and update prompt #124

merged 3 commits into from
May 9, 2024

Conversation

diracdeltas
Copy link
Member

No description provided.

@diracdeltas diracdeltas marked this pull request as ready for review May 9, 2024 04:39
@diracdeltas diracdeltas changed the title Refactor common explainPatch code into utility func DRAFT: refactor/update prompt May 9, 2024
@diracdeltas diracdeltas requested a review from thypon May 9, 2024 05:03
@diracdeltas diracdeltas changed the title DRAFT: refactor/update prompt refactor and update prompt May 9, 2024
thypon
thypon reviewed May 9, 2024
View reviewed changes
src/utils.js Outdated
<list_of_security_hotspots> // Describe locations for possible vulnerabilities in the change, ordered by risk. Do not include a vulnerability unless it is likely to present a real security risk. If there are none, omit this section.

### Other Issues
<list_of_issues> // Describe any other major issues with this code such as license incompatibilities introduced by a dependency. If there are none, omit this section.
Copy link
Member

Choose a reason for hiding this comment

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

  • `s/ such as license incompatibilities introduced by a dependency//g'
  • `s/ any other major/ any other major non-security//g'

src/utils.js Outdated
}

response = response.replaceAll('### Changes', '<details>\n<summary><i>Changes</i></summary>\n\n### Changes')
response = response.replaceAll('### Security Hotspots', '</details>\n\n### Security Hotspots')
Copy link
Member

Choose a reason for hiding this comment

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

Since Security Hotspots section can now be omitted, this should be adapted, to get the first ### after the ### Changes section, otherwise it breaks the formatter.

This is custom github syntax, and the AI are not aware of

src/utils.js
for (let i = 0; i < models.length; i++) {
try {
model = models[i]
response = await getResponse(userPrompt, model)
Copy link
Member

Choose a reason for hiding this comment

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

Welcome to ruby JS

@brave brave deleted a comment from github-actions bot May 9, 2024
@brave brave deleted a comment from github-actions bot May 9, 2024
@brave brave deleted a comment from github-actions bot May 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

openai debug - [puLL-Merge] - brave/pull-merge@124

Description

This pull request introduces a significant refactoring across various files within the repository to standardize the implementation of model-based patch explanations for GitHub pull requests. The primary motivation for these changes seems to be the centralization of common code into utility functions, enhancement of maintainability, and improvement of code reuse across different models such as Anthropic, Bedrock, and OpenAI implementations.

Possible Issues

  1. Error Handling Scope: The refactoring introduces a centralized error handling mechanism within the explainPatchHelper function. If individual model-specific implementations require unique error handling strategies, this centralized approach might restrict flexibility.
  2. Debugging Transparency: With the encapsulation of debug logging within explainPatchHelper, ensuring that adequate and clear debug information is provided could be challenging, especially when dealing with model-specific nuances.
  3. Performance Considerations: The refactoring does not explicitly consider the optimization of model invocations or API calls, which could impact performance depending on the number of models tried and the size of the data being processed.

Security Hotspots

  1. Model Data Handling: Caution should be maintained regarding how data is handled when passed to external models, especially with direct integration without sufficient sanitization or validation. Although the PR does not explicitly introduce vulnerabilities, improper future modifications could lead to security risks.
Changes

Changes

src/utils.js

  • Introduced SYSTEM_PROMPT and explainPatchHelper: Centralized the system prompt and the method handling the interaction with models, reducing repetition and increasing maintainability.

src/anthropicExplainPatch.js, src/bedrockExplainPatch.js, src/openaiExplainPatch.js

  • Removed repetitive code blocks and utilized SYSTEM_PROMPT and explainPatchHelper for handling the core functionality.
  • Simplified model invocation and error handling by leveraging the newly created utility functions.
  • Enhancements in logging and handling of API responses, with emphasis on readability and debugging.

General Implication Across Files

  • Common changes include removing local implementations of the system prompt and substituting with SYSTEM_PROMPT from utils.
  • Transitioned from individual to unified handling of model responses and errors through explainPatchHelper.
  • Improved consistency and development efficiency by reducing boilerplate code and centralizing common functionality.

@brave brave deleted a comment from github-actions bot May 9, 2024
@brave brave deleted a comment from github-actions bot May 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

anthropic debug - [puLL-Merge] - brave/pull-merge@124

Description

This PR refactors the code for explaining patches using different AI models (Anthropic, Bedrock, and OpenAI). It extracts common functionality into a new explainPatchHelper function in utils.js, which is then used by each of the model-specific explain patch functions. The system prompt and some other repeated code is also moved into constants in utils.js.

The main motivation seems to be to reduce code duplication and improve maintainability by centralizing shared logic.

Changes

Changes

  • src/anthropicExplainPatch.js:
    • Import SYSTEM_PROMPT and explainPatchHelper from utils.js
    • Remove repeated code that is now in explainPatchHelper
    • Use explainPatchHelper to generate the response, passing a callback to handle model-specific logic
  • src/bedrockExplainPatch.js:
    • Similar changes as in anthropicExplainPatch.js
    • Use explainPatchHelper with a Bedrock-specific callback
  • src/openaiExplainPatch.js:
    • Similar changes as in anthropicExplainPatch.js
    • Use explainPatchHelper with an OpenAI-specific callback
  • src/utils.js:
    • New file containing shared code
    • SYSTEM_PROMPT constant holds the system message for the AI
    • explainPatchHelper function encapsulates the common logic for iterating over models, handling errors, and formatting the response

The main changes are nicely organized by file. The core explain patch logic for each model API remains in its respective file, while the common code is factored out to utils.js.

@brave brave deleted a comment from github-actions bot May 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

bedrock debug - [puLL-Merge] - brave/pull-merge@124

Description

This PR refactors the explainPatch functions for Anthropic, Bedrock, and OpenAI to extract common functionality into a shared explainPatchHelper function in a new utils.js file. The SYSTEM_PROMPT is also moved to utils.js to be shared across the implementations. This change reduces code duplication and improves maintainability.

Changes

Changes

  • src/anthropicExplainPatch.js:

    • Imports SYSTEM_PROMPT and explainPatchHelper from utils.js
    • Simplifies the function by delegating to explainPatchHelper and passing a callback to handle model-specific logic
    • Removes the logic to iterate over models and error handling, which is now in explainPatchHelper
    • Removes the code to add <details> tags around the Changes section, which is now handled in explainPatchHelper

  • src/bedrockExplainPatch.js:

    • Similar changes as anthropicExplainPatch.js
    • Imports SYSTEM_PROMPT and explainPatchHelper from utils.js
    • Simplifies by delegating to explainPatchHelper

  • src/openaiExplainPatch.js:

    • Similar changes as the other files
    • Imports SYSTEM_PROMPT and explainPatchHelper from utils.js
    • Simplifies by delegating to explainPatchHelper

  • src/utils.js:

    • New file that contains shared functionality
    • Exports SYSTEM_PROMPT constant
    • Exports explainPatchHelper function which:
      • Takes patchBody, owner, repo, models, debug and a getResponse callback
      • Handles iterating over models and error handling
      • Adds <details> tags around the Changes section of the response
      • Appends the model name used to generate the response

@thypon thypon merged commit e5a2dd4 into main May 9, 2024
8 checks passed
@thypon thypon deleted the fix/refactor branch May 9, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thypon thypon thypon left review comments

Assignees
No one assigned
Labels
puLL-Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@diracdeltas @thypon