Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

/review #748

Closed
wants to merge 44 commits into from
Closed

/review #748

wants to merge 44 commits into from

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Sep 10, 2023

Resolves #746

invoked via /review

The implemented prompts differ from the original spec but after experimenting with variations, I believe the current setup provides uniform formatting and covers all requirements of the spec.

Keyrxng added 25 commits August 23, 2023 17:38
    Simple prompt engineering, one response per user per question.
better initial prompt
original context indexable across repos, issues and pull requests
comments etc
waiting for more input
pulls from sources using the hashtag in issue body
total token usage now displayed
feed gpt the spec and diff for it to analyze
will only spec check once now fixed tsc issues
sort import
allow it to be set in org and repo config
reset default
no longer undefined
set to 8k in config 4k by default
improved pr-review and context for both ask and pr-review commands
added gpt pr review
@netlify
Copy link

netlify bot commented Sep 10, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit f1400cc
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/650bf1bdb191150008e68d8d
😎 Deploy Preview https://deploy-preview-748--ubiquibot-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Keyrxng Keyrxng marked this pull request as ready for review September 10, 2023 21:23
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Interesting prompts. Seems like a lot of experimentation was put into them. Understanding purely via code review is tough for that but I would love to see QA examples to see those prompts in action.

src/handlers/comment/handlers/index.ts Outdated Show resolved Hide resolved
src/handlers/comment/handlers/review.ts Outdated Show resolved Hide resolved
src/handlers/comment/handlers/review.ts Outdated Show resolved Hide resolved
src/handlers/comment/handlers/review.ts Outdated Show resolved Hide resolved
src/handlers/comment/handlers/review.ts Outdated Show resolved Hide resolved
src/handlers/comment/handlers/review.ts Outdated Show resolved Hide resolved
src/helpers/gpt.ts Outdated Show resolved Hide resolved
@0x4007
Copy link
Member

0x4007 commented Sep 14, 2023

It could be interesting to plug this into CI on push so that it won't pass until the bot thinks it passed. Until we implement automatically closing pull requests due to poor performance though, we should not automatically invoke the /review.

@0x4007 0x4007 mentioned this pull request Sep 14, 2023
@whilefoo whilefoo changed the title Review /review Sep 17, 2023
@0xcodercrane
Copy link
Contributor

It could be interesting to plug this into CI on push so that it won't pass until the bot thinks it passed. Until we implement automatically closing pull requests due to poor performance though, we should not automatically invoke the /review.

good point. we can create an ai-review ci/cd running on each pull request.

@0x4007
Copy link
Member

0x4007 commented Sep 20, 2023

I'm really looking forward for this one to get merged in because I think it will be able to help us merge the other pull requests faster. @Keyrxng any updates?

By the way, https://github.com/Keyrxng/UbiquityBotTesting/pull/41#issuecomment-1719534993 is my favorite review result.

src/configs/shared.ts Outdated Show resolved Hide resolved
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 20, 2023

any updates?

I felt that I was waiting for you guys but I will stop work on Blame and just ensure everything is operating inline with the favourite you mentioned which I'm sure it is. So long as it is my work with this for now is done unless there are further requests. Will let you know in a couple hours.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 20, 2023

So the response is short and sweet if it's meeting the spec while when it's failing it's going into depth with things and suggesting how to meet the spec or what is missing from the current implementation.

Done my best to remove any type of overview or listings, I think that ratio you mentioned is better in the more recent stuff than the one you said was your favourite so far.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 20, 2023

What would be so ideal is if it could be tested on closed prs or something as that amount of real data would be so much better to work with than my shitty gpt-assisted test issues 🤣 Hope it's looking better tho, lmk and I'll wrap this up tomorrow.

better signal to noise more concise
Remove blockquote from footer
@0x4007 0x4007 requested a review from 0xcodercrane September 21, 2023 06:57
@0x4007
Copy link
Member

0x4007 commented Sep 21, 2023

Does it make sense to eventually turn all the AI functions into -background functions?

https://docs.netlify.com/functions/background-functions/

@0xcodercrane rfc

@@ -8,6 +8,8 @@
"disable-analytics": false,
"comment-incentives": false,
"register-wallet-with-verification": false,
"openai-api-key": null,
"openai-max-tokens": 8000,
Copy link
Member

Choose a reason for hiding this comment

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

This is still 8000

Copy link
Contributor

Choose a reason for hiding this comment

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

1/ Why put the null value in default.json here?
2/ @pavlovcik What's your recommendation value? If you put it in the comment, I can directly push to speed up the merge.

@0xcodercrane
Copy link
Contributor

Does it make sense to eventually turn all the AI functions into -background functions?

https://docs.netlify.com/functions/background-functions/

@0xcodercrane rfc

The background function is a new feature of netlify and I think it's worth to take the 15 mins execution timeout benefit of that feature. but my concern from their docs is the pricing plan. they just say it may not be available on all the pricing plans.

@molecula451 molecula451 mentioned this pull request Sep 23, 2023
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 14, 2023

Okay I'm back in the game

All good to pull things from this pr and start with a clean commit history from the most recent dev branch, seems cleaner and I can avoid dealing with all of the conflicts?

@Keyrxng Keyrxng mentioned this pull request Oct 14, 2023
@Keyrxng Keyrxng closed this Oct 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AI: /review
3 participants