-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
ProposalPolice™ GH Actions Workflow #41038
Conversation
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Not much for a C+ to test here so I think we'll forego it this time @DylanDylann |
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.
@ikevin127 how would you feel about implementing this in TypeScript and using ts-node instead? Especially since our manual testing is kind of limited here that seems like a nice little safety net to make changing this code safer.
@roryabraham Sure, I'll get on this and hopefully will have something by tomorrow EOD. |
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.
The way to implement this that would rock the boat less would be to:
- Remove the added package.json
- add any missing dependencies (i.e: openai) to the main package.json as dev dependencies
- Move the scripts from
scripts/proposal-police
to.github/actions/javascript
, create a GitHub Action for each (or maybe just one that you control the behavior of with inputs). - Compile the JS action with
ncc
by adding it tobuildActions.sh
I'm not sure that's the best way to configure all the GitHub Actions in this repo, but it is the established way. So I think we should either continue with that pattern or create a S/P/S statement to change it. Hopefully that makes sense 👍🏼
Also, do you think it would be valuable to add some automated tests here? Asking earnestly, not rhetorically |
Yes, I think it would be a good thing. Will look into it after the TS conversion and changes from #41038 (review). |
@roryabraham Did this, just pushed the changes to align with the established way we handle GH actions. Is there any way we could test if this is working so far ? I'm not entirely sure the import paths would work once compiled / run started. Once we confirm that it's working as expected, I can move on to writing some tests. |
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.
Hey @ikevin127, thanks for making the changes so far. It seems like you're a bit confused by my previous suggestions, so let me try to clarify.
There is a difference between GitHub Workflows and GitHub Actions. It's confusing that collectively the CI/CD product offered by GitHub is called GitHub Actions
😂
Here's my 2 minute explanation... Workflows are triggered by events, and use the syntax you're using in your yaml files to list a series of jobs. Each job runs on it's own machine, and is comprised of a series of steps. There are also callable workflows that can be triggered from other workflows, and they are like a job in the calling workflow and also run on their own machine (or set of machines, if the callable workflow has multiple jobs).
Meanwhile, Actions are essentially callable scripts that can be called at the step level. This means that Actions are executed on the machine that calls them. There are composite actions and javascript actions (and also docker actions, but we don't use any in this repo).
Workflows and Actions are both defined in yaml, but have different syntaxes:
Now, let's talk about what I'd like you to do here:
- You should create simple action metadata files for each of the TS actions you wrote. example.
- You should also run
npm i && npm run gh-actions-build
. This will use vercel/ncc to process the TS files you wrote and create a bundled executable Node.js script in pure JS. This means that you no longer need to run a node install to run that script - you just need Node. it has all the dependencies built-in. These massive files will be at.github/actions/javascript/proposalPoliceComment/index.js
and.github/actions/javascript/proposalPoliceCommentEdit/index.js
, respectively. Commit them and push them to this PR.- Curious why we have to compile these actions? Read on
- Then, reintroduce GitHub Workflow(s) in
.github/workflows
that are triggered by adding or comment to or editing a comment on an issue. These workflows can call your reusable TS actions you created.
Hopefully that helps clarify things a bit. I didn't dive into a review of what we've got here because of these kind of fundamental misunderstandings. But fret not, we shall get there 🙂
Regarding tests, feel free to check out the README in the |
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.
also npm ci
is failing, I think because you need to reinstall node_modules
and commit the diff in package-lock.json
.github/actions/javascript/proposalPoliceComment/proposalPoliceComment.ts
Outdated
Show resolved
Hide resolved
.github/actions/javascript/proposalPoliceComment/proposalPoliceComment.ts
Outdated
Show resolved
Hide resolved
.github/actions/javascript/proposalPoliceComment/proposalPoliceComment.ts
Outdated
Show resolved
Hide resolved
.github/actions/javascript/proposalPoliceComment/proposalPoliceComment.ts
Outdated
Show resolved
Hide resolved
.github/actions/javascript/proposalPoliceComment/proposalPoliceComment.ts
Outdated
Show resolved
Hide resolved
I applied the requested changes and after syncing with main and performing a clean install looks like Not sure how should I fix that one, should I run |
Yep, gh-actions-build and commit diff in index.js. Since they're bundled with all dependencies inline if you change dependencies or shared libs they can all change, even if you didn't touch the source code for that action |
Ah yeah good catch and thanks for testing 👍 |
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.
Looks great now 👍🏼
@marcochavezf back to you, what do we need to do to get this merged?
Circleing back to the Important part of this #35108 (comment) where I detailed exactly how to setup the AI assistant part 🚀 |
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.
LGTM! Just a minor requested change (I think)
} | ||
|
||
// Verify that the comment is not empty | ||
if (!payload.comment?.body.trim()) { |
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.
Can we also check if the comment begins with # Proposal
? So we can only check the comments that contain a proposal, otherwise I think we can trigger the OpenAI API in all the comments
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.
@marcochavezf This is handled by the AI assistant on both new comments which checks if a proposal is following the template and also on proposal edits to check for significant changes.
otherwise I think we can trigger the OpenAI API in all the comments
That's correct as per AI assistant instructions currently we run it on all comments of issues with Help Wanted
label and if no proposal template is detected as part of the comment, the AI is instructed to respond in short with "NO_ACTION"
and the bot don't do anything.
Can we also check if the comment begins with
# Proposal
?
This wouldn't be a good idea because currently for if AI assistant detects a proposal has significant changes, it will add a line like: Edited by proposal-police: This proposal was edited at 2024-06-22 22:28:40 UTC.
at the top of the proposal which would break the logic proposed by you (see example in ikevin127/expensify-proposal-testing#1 (comment)).
For details like this we decided to rely on the AI assistant via instructions because we could have users add different proposal template markdown (or just text) which would still be valid like: Proposal
, # Proposal
, ## Proposal
.
If we take a look at the given AI instructions you'll notice that we made sure to instruct for variations like these and also for if any other things are added above / below the proposal like the Edited by proposal-police...
AI would look at the whole comment and if the proposal follows the template (+/- acceptable markdown / text variations), the AI would know about this and only respond if the proposal template is actually not followed (different RCA / solution / alt solution headings, ex: ikevin127/expensify-proposal-testing#1 (comment)).
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 we want to optimize this and not run the AI on every new comment, I guess we can be very strict and check if new issue comment contains # Proposal
and only if true we run the AI, otherwise don't run at all ? But this would have the following edge case:
- if user posts proposal using say
## Proposal
or plain textProposal
and AI won't run on it -> nothing will happen (AI won't post the "you did not follow the proposal template" comment)
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 we just check if the comment contains the word proposal (case-insensitive)? The problem is that we generate a considerable number of issues per month, and each issue could have multiple comments where it's not necessary to send an OpenAI API request
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.
Ya, I think that can definitely be done in order to reduce the amount of times we call the AI, therefore reduce costs. Will get on this tomorrow and make the change.
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.
@marcochavezf Done, synced w/ main and added the discussed change to make sure that the comment body includes the keyword Proposal
case-sensitive.
Discussing internally the costs before setting the API keys as env variables |
Asked to set up the API keys |
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.
The keys are set, but are set as PROPOSAL_POLICE_API_KEY
and PROPOSAL_POLICE_ASSISTANT_ID
GITHUB_TOKEN: | ||
description: 'Auth token for New Expensify Github' | ||
required: true | ||
OPENAI_API_KEY: |
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.
Let's update this to PROPOSAL_POLICE_API_KEY
OPENAI_API_KEY: | |
PROPOSAL_POLICE_API_KEY: |
OPENAI_API_KEY: | ||
description: 'OpenAI API key for accessing AI services' | ||
required: true | ||
OPENAI_ASSISTANT_ID: |
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.
And this one to PROPOSAL_POLICE_ASSISTANT_ID
OPENAI_ASSISTANT_ID: | |
PROPOSAL_POLICE_ASSISTANT_ID: |
.github/libs/OpenAIUtils.ts
Outdated
private static assistantID: string; | ||
|
||
static init(apiKey?: string, assistantID?: string) { | ||
const key = apiKey ?? getInput('OPENAI_API_KEY', {required: true}); |
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.
const key = apiKey ?? getInput('OPENAI_API_KEY', {required: true}); | |
const key = apiKey ?? getInput('PROPOSAL_POLICE_API_KEY', {required: true}); |
.github/workflows/proposalPolice.yml
Outdated
uses: ./.github/actions/javascript/proposalPoliceComment | ||
with: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} |
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.
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} | |
PROPOSAL_POLICE_API_KEY: ${{ secrets. PROPOSAL_POLICE_API_KEY }} |
.github/libs/OpenAIUtils.ts
Outdated
throw new Error('Could not initialize OpenAI: No key provided.'); | ||
} | ||
this.ai = new OpenAI({apiKey: key}); | ||
this.assistantID = assistantID ?? getInput('OPENAI_ASSISTANT_ID', {required: true}); |
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.
this.assistantID = assistantID ?? getInput('OPENAI_ASSISTANT_ID', {required: true}); | |
this.assistantID = assistantID ?? getInput('PROPOSAL_POLICE_ASSISTANT_ID', {required: true}); |
.github/workflows/proposalPolice.yml
Outdated
with: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} | ||
OPENAI_ASSISTANT_ID: ${{ secrets.OPENAI_ASSISTANT_ID }} |
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.
OPENAI_ASSISTANT_ID: ${{ secrets.OPENAI_ASSISTANT_ID }} | |
PROPOSAL_POLICE_ASSISTANT_ID: ${{ secrets. PROPOSAL_POLICE_ASSISTANT_ID }} |
@marcochavezf I put this on HOLD because @mallenexpensify suggested copy changes related to the proposal enforcement comment on ND discussion. Motivation for this can be found in ND discussion, stating:
Here are the updated instructions ↩️Simply replace all current OpenAI assistant instructions with the updated one from above and once confirmed, the HOLD can be removed and PR can be merged. Note: The only change was on BOT ACTIONS -> 1. NEW COMMENTS where I changed the response copy in quotes from:
No javascript code changes were required for this change. |
Merged w/ main and solved conflicts. |
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.
Nice! The instructions were updated. Let's put it into action!
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Looks like we got it on the first try 🎉 (below are some screenshots of the workflow running as expected) |
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.0.14-1 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.14-6 🚀
|
Details
The ProposalPolice™ will help promote higher quality proposals and a more informed and trust-based review process 🤝, improving baseline fairness and allowing community members and Expensify employees to better focus on getting shit done!
For more details check out:
Fixed Issues
$ #35108
PROPOSAL: #35108 (comment)
Tests
N/A
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
N/A