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

Adopt chat-extension-utils #86

Merged
merged 4 commits into from
Dec 2, 2024
Merged

Conversation

roblourens
Copy link
Member

@roblourens roblourens commented Nov 21, 2024

What do you think?

@amunger @eleanorjboyd @joshspicer

I think the big string prompt is a bit ugly, I'm thinking about supporting PromptElements for that.

@amunger
Copy link
Contributor

amunger commented Nov 21, 2024

It's much harder to tell how tools will be used and how they are used in the prompt. I assume they just get run and the result is inserted into the prompt, but maybe harder to troubleshoot or adjust. Definitely nicer that I don't need to worry about caching tool results though - that part felt like the most obvious part that everyone would need.

LGTM overall.

Copy link
Member Author

@roblourens roblourens left a comment

Choose a reason for hiding this comment

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

Yeah, but the idea is that you shouldn't have to do much troubleshooting, the library should do the right thing. Is there something that you expect having to adjust?

You do bring up a good point that it should give you a way to see the real prompt/request, maybe starting up the trace utility.

@amunger
Copy link
Contributor

amunger commented Nov 26, 2024

Yeah I think I would just want that visibility to confirm that it is getting the context correctly.

@roblourens
Copy link
Member Author

I did add something to show the trace utility, it prints a message inline. I also opened this microsoft/vscode-prompt-tsx#134

@roblourens roblourens marked this pull request as ready for review December 2, 2024 18:57
@roblourens
Copy link
Member Author

Any other thoughts on this? It doesn't have to be merged, but it would be cool if it was

@eleanorjboyd eleanorjboyd merged commit 0e009b9 into main Dec 2, 2024
2 checks passed
@eleanorjboyd eleanorjboyd deleted the roblou/appropriate-landfowl branch December 2, 2024 19:15
@roblourens
Copy link
Member Author

roblourens commented Dec 2, 2024

Looks like this was depending on a version of the library that wasn't published yet, since I was working with npm link- I'll fix that up. I am a little scared that you were willing to merge it without trying it but I appreciate your trust! 😄

@eleanorjboyd
Copy link
Member

will try it next time haha

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.

4 participants