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

Use the connect protocol to generate unary AI completions #1559

Merged
merged 10 commits into from
Aug 26, 2024

Conversation

jlewi
Copy link
Contributor

@jlewi jlewi commented Aug 19, 2024

@jlewi jlewi changed the title Jlewi/connect Use the connect protocol to generate completions Aug 19, 2024
@jlewi jlewi changed the title Use the connect protocol to generate completions Use the connect protocol to generate unary AI completions Aug 19, 2024
@jlewi
Copy link
Contributor Author

jlewi commented Aug 19, 2024

Need to merge #1554 and then rebase this PR.
We should be able to drop all the changes prior to commit 7c3924a

@jlewi jlewi marked this pull request as ready for review August 22, 2024 00:44
@jlewi
Copy link
Contributor Author

jlewi commented Aug 22, 2024

@sourishkrout this is ready for review.

@sourishkrout
Copy link
Member

@sourishkrout this is ready for review.

Sounds good. Feel free to add me as one of the reviewers. A Slack bot reminds me twice a day about pending reviews.

@sourishkrout sourishkrout self-requested a review August 22, 2024 15:25
@@ -74,6 +74,8 @@ vi.mock('../../src/extension/grpc/runner/v1', () => ({}))
test('initializes all providers', async () => {
const configValues = {
binaryPath: 'bin',
// This is needed for the AIManager to initialize.
'runme.aiBaseURL': 'http://localhost:8080/api',
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest using a different port, 8080, which is over-used. Fine inside container images, but running locally, there's a high likelihood another process bound the port locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good Idea. I changed the default to 8877.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Good choice, @jlewi!

Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

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

✅ LGTM

Copy link

sonarcloud bot commented Aug 26, 2024

@sourishkrout sourishkrout merged commit f6a63bf into main Aug 26, 2024
3 checks passed
@sourishkrout sourishkrout deleted the jlewi/connect branch August 26, 2024 22:46
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.

2 participants