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

Command LLM #186

Merged

Conversation

whilefoo
Copy link
Contributor

@whilefoo whilefoo commented Nov 3, 2024

Resolves #166

@whilefoo
Copy link
Contributor Author

whilefoo commented Nov 3, 2024

I have a dilemma about the command interface.
I added a command property in the input object that has command name and command parameters. The name and parameters are given by the LLM when the user tags the bot.

I'm guessing we still wanna keep the /command something 10 so I'm thinking between

  1. letting the LLM also infer this way of calling a command but it might not always be successful because there is less context than a user using natural language to express what they want
  2. leaving the command input empty and the plugin itself has to decode the command from the comment payload like it has done now
  3. removing this way of calling the command but if the LLM can't figure out how to call a command, you basically can't do anything

First option seems better because there's only way to receive commands and it's already parsed by the LLM so the plugin doesn't need to do any parsing, but might be inaccurate at times
Second option makes sure that the direct way of calling a command is robust and not prone to hallucinations by LLM

@0x4007 @gentlementlegen

@gentlementlegen
Copy link
Member

Second option seems proper to me because it is what we use for all the plugins so the parser is ready without any change, making maintenance easier.

@0x4007
Copy link
Member

0x4007 commented Nov 4, 2024

@sshivaditya2019 perhaps you can offer some pointers related to function calling

@whilefoo
Copy link
Contributor Author

whilefoo commented Nov 5, 2024

I will leave it for now since every plugin already supports parsing, we can add later if we want

Comment on lines 21 to 26
const dispatchWorkflow = jest.fn();
jest.mock("../src/github/utils/workflow-dispatch", () => ({
//...(jest.requireActual("../src/github/utils/workflow-dispatch") as object),
getDefaultBranch: async () => "main",
dispatchWorkflow: dispatchWorkflow,
}));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gentlementlegen I've been fighting with Jest for hours...do you might know why is mocking working only outside the test function. In sdk.test.ts I put it in the test function and it worked but not here.

Also getDefaultBranch returns undefined when it gets called by issueCommentCreated and I don't understand why

Copy link
Member

@gentlementlegen gentlementlegen Nov 8, 2024

Choose a reason for hiding this comment

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

We have moved to ESM so I'd suggest trying unstable_mock instead. As such, imports should happen after the mock, since it is hoisted.
https://jestjs.io/docs/ecmascript-modules

(And I second you, it's a pain in the ass to deal with all this CJS ESM compatibility, I am having nightmares within text-conversation-rewards`)

Copy link
Member

Choose a reason for hiding this comment

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

Update: it broke in the SDK when I moved it too. It is usually easier to fake network calls than the actual package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not even sure if kernel tests are running with CJS or ESM because if I add experimental vm modules and unstable mock, it says that import must be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even though the module issue-comment-created is dynamically imported in the test function, it's still importing original dispatchWorkflow and not mocked

Copy link
Member

Choose a reason for hiding this comment

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

This can happen if the module you are trying to lock is previously imported by another module first, in such case the mock won't work. I've seen such scenario within the SDK.

Copy link
Contributor Author

@whilefoo whilefoo Nov 9, 2024

Choose a reason for hiding this comment

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

It seems the issue was that jest caches imported modules and I imported issue-comment-created in 3 test cases but only mocked dispatchWorkflow in the 2nd test case, but jest cached the first import so the mock didn't work. I had to use jest.resetModules() to reset the cache.
Thanks for the help!

Copy link
Member

Choose a reason for hiding this comment

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

Glad you figured it out. Jest has been giving me headaches a lot lately particularly when ESM came into play.

@whilefoo
Copy link
Contributor Author

@gentlementlegen I've just realised that separating SDK makes development more difficult, I made changes to Manifest but now it's in SDK package so I need to use bun link and I have to build the SDK every time I make a change.
Also decoding manifest schema gives some TS errors about types not matching even though Typebox version is the same in kernel and SDK

@gentlementlegen
Copy link
Member

@whilefoo The types not matching was there before somehow, not sure about the cause.

What I usually do is to build with a --watch so I don't have to think of it. If that is too burdensome, we can consider merging it back, but it has to be perfectly separated otherwise we will have the circular reference again which will break ncc compilation.

@whilefoo whilefoo marked this pull request as ready for review November 10, 2024 22:19
@whilefoo
Copy link
Contributor Author

QA: ubiquibot-whilefoo-testing/testing#7

@gentlementlegen
Copy link
Member

@whilefoo Looks cool. I wanted to try some commands, I guess the bot was not able to execute them which is ok but I got no feedback on them: ubiquibot-whilefoo-testing/testing#7 (comment) should there be some error message displayed in such case?

@whilefoo
Copy link
Contributor Author

@whilefoo Looks cool. I wanted to try some commands, I guess the bot was not able to execute them which is ok but I got no feedback on them: ubiquibot-whilefoo-testing/testing#7 (comment) should there be some error message displayed in such case?

I was running it locally on my laptop and I think it went to sleep so it stopped replying

@0x4007
Copy link
Member

0x4007 commented Nov 12, 2024

QA: ubiquibot-whilefoo-testing/testing#7

This is amazing for demos to see let's merge it if you think it's in a good spot to.

Really looking forward to this natural language interface either on the copilot chat window on GitHub web desktop / iOS and/or our telegram bot.

package.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Format

const body = context.payload.comment.body.trim();
if (/^\/help$/.test(body)) {
const body = context.payload.comment.body.trim().toLowerCase();
if (body.startsWith(`@ubiquityos`)) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should offer a short hand syntax.

As I understand @UbiquityOS wouldn't automatically populate on the GitHub UI.

Mixed thoughts on this idea, but it might be more ergonomic to use with something like @U or @os

Also this next idea could be out of scope but wondering if we should intercept all comments and run commands on behalf. Would be perfect if we could handle those assigns for the newcomers asking to work on tasks for example.

Copy link
Contributor Author

@whilefoo whilefoo Nov 12, 2024

Choose a reason for hiding this comment

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

I wonder if we should offer a short hand syntax.

As I understand @UbiquityOS wouldn't automatically populate on the GitHub UI.

Mixed thoughts on this idea, but it might be more ergonomic to use with something like @U or @os

Yes, it's a bit cumbersome to always type out @UbiquityOS so a short hand tag would be great but we will be tagging another person :D

Also this next idea could be out of scope but wondering if we should intercept all comments and run commands on behalf. Would be perfect if we could handle those assigns for the newcomers asking to work on tasks for example.

Do you mean without the tag? For example if the users says "how can I start this task?" the router should run the start command?
It sounds good in theory but in practice it might trigger it even when not intended to and will consume OpenAI API a lot which will result in higher cost.

Copy link
Member

Choose a reason for hiding this comment

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

It might be interesting to have some type of local logic (perhaps non LLM) to determine if the comment is likely asking for some type of action. Accuracy can be pretty low and in theory it could still cut quite a bit of costs.

Also with mini models costs might already be cheap enough for it to be feasible. We would need to run projections I suppose.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @whilefoo that tagging another person is not good. Maybe we should create a @UbiquityOS account to get the auto-completion when typing the name.

- **Tagged Natural Language**: Interpret the "comment" field provided in JSON. Users will mention you with "@UbiquityOS", followed by their request. Infer the intended command and parameters based on the "comment" content.

- **Action**: Map the user's intent to one of your available functions. When responding, use the "author", "repositoryOwner", "repositoryName", and "issueNumber" fields as context if relevant.
`,
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 a high quality prompt good job

@whilefoo
Copy link
Contributor Author

whilefoo commented Nov 12, 2024

This is amazing for demos to see let's merge it if you think it's in a good spot to.

I have to modify and make PR for each plugin to accommodate for the new interface so only then we can merge it

@0x4007
Copy link
Member

0x4007 commented Nov 15, 2024

This is amazing for demos to see let's merge it if you think it's in a good spot to.

I have to modify and make PR for each plugin to accommodate for the new interface so only then we can merge it

Can't this be done async? Lets just merge these and then fix the plugins later? Or will kernel be broken?

Copy link

@whilefoo, this task has been idle for a while. Please provide an update.

@whilefoo
Copy link
Contributor Author

Can't this be done async? Lets just merge these and then fix the plugins later? Or will kernel be broken?

Unfortunately not, plugins won't run because of this change

Copy link

@whilefoo, this task has been idle for a while. Please provide an update.

@gentlementlegen
Copy link
Member

@whilefoo could you update the .dev.vars.example with the OPEN_API_KEY?

Copy link

@whilefoo, this task has been idle for a while. Please provide an update.

@0x4007
Copy link
Member

0x4007 commented Nov 25, 2024

What's the status on updating all the plugins? Should we merge?

@gentlementlegen
Copy link
Member

Every plugin has been merged, two remain with deployment issues that are being fixed. Afterwards this will be merged.

@gentlementlegen
Copy link
Member

gentlementlegen commented Nov 26, 2024

Current only https://github.com/ubiquity-os-marketplace/text-vector-embeddings/ is not deployed but was not working before anyway, so I am merging this, expect a fix for the plugin today.

@gentlementlegen gentlementlegen merged commit a2726d7 into ubiquity-os:development Nov 26, 2024
3 checks passed
@ubiquity-os-beta ubiquity-os-beta bot mentioned this pull request Nov 26, 2024
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.

LLM Command Router
3 participants