-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Added LLM-based issue summarizer. #1927
base: main
Are you sure you want to change the base?
Conversation
.github/workflows/summarize.yml
Outdated
- name: Summarize issues | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
MODEL_ID : "anthropic.claude-3-5-sonnet-20240620-v1:0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is going to be hard-coded, it can just go in your Action js script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, will migrate in next revision. Originally the idea was to be able to hot-swap models if needed, but I've noticed the inputs vary too much from model to model to be feasible.
|
||
console.log("Prompting LLM with:\n" + JSON.stringify(modelInput)); | ||
|
||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- too much going on in a single try/catch
- if you just catch the error, log it, and rethrow -- you don't need the try/catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, revising in next revision.
.github/actions/summarizer/index.js
Outdated
messages[0].content.push({ | ||
type: "text", | ||
text: ` | ||
Human: ${prompt} | ||
Assistant: | ||
` | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you move this into the declaration of messages
above?
.github/actions/summarizer/index.js
Outdated
let commentLog = "Comment Log:\n"; | ||
|
||
commentLog += `${issue.user.login} created the issue:\n "${issue.body}"\n` | ||
|
||
for (const comment of comments) { | ||
if( | ||
(comment.user.login != "github-actions[bot]") && | ||
(!comment.body.startsWith("/")) | ||
){ | ||
commentLog += `${comment.user.login} says:\n"${comment.body}"\n` | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building the string like this is a bit clunky, maybe try something like:
const commentLog = comments.filter(c => c.user.login != "github-actions[bot]")
.map(c => `${c.user.login} says: ...`)
.join('\n');
const authorized = ["OWNER", "MEMBER"].includes(payload.comment.author_association); | ||
// Ensure that invoker is either a Owner or member of awslabs / amazon-eks-ami | ||
if (!authorized) { | ||
console.log(`Comment author is not authorized: ${author}`); | ||
return; | ||
} | ||
console.log(`Comment author is authorized: ${author}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to extend the existing bot vs reimplementing the authz here
console.log(`Comment author is authorized: ${author}`); | ||
|
||
// Split the command into parts | ||
const parts = process.env.COMMENT_BODY.trim().split(' '); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the comment has more than just the command in it? what if it has multiple commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, it requires that you only type a single command. Working on moving the summarizer over to the existing bot.
Issue #, if available:
N/A
Description of changes:
Added a GitHub Action that will summarize issues via:
/summarize
/summarize 123
/summarize bob dotfiles 123
Example Output:
The bot utilizes AWS Bedrock to trigger a
InvokeModelCommand
to Claude 3.5 Sonnet. In order to achieve this, a few things will have to be configured on a AWS Account:sts:AssumeRoleWithWebIdentity
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Testing Done
See this guide for recommended testing for PRs. Some tests may not apply. Completing tests and providing additional validation steps are not required, but it is recommended and may reduce review time and time to merge.