-
Notifications
You must be signed in to change notification settings - Fork 68
fix: Add conversational context to interactive CLI chat #465
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @nithish-95, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue where the interactive chat in the command-line interface lacked conversational memory. By implementing a robust mechanism to store and transmit the full exchange of messages, the system can now provide a more natural and coherent chat experience, ensuring the underlying model retains context throughout the conversation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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 there - I've reviewed your changes - here's some feedback:
- The new
Messagetype innim.gois very similar in shape todesktop.OpenAIChatMessage; consider reusing or sharing a common chat message struct to avoid duplication and keep the OpenAI-compatible payload types consistent across the codebase. - Relying on the mutable package-level
nimDefaultPort(especially modified in tests) makes the behavior more implicit; it may be more robust to inject the base URL/port intochatWithNIM(or into a small NIM client struct) so tests and callers can configure endpoints without altering global state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Message` type in `nim.go` is very similar in shape to `desktop.OpenAIChatMessage`; consider reusing or sharing a common chat message struct to avoid duplication and keep the OpenAI-compatible payload types consistent across the codebase.
- Relying on the mutable package-level `nimDefaultPort` (especially modified in tests) makes the behavior more implicit; it may be more robust to inject the base URL/port into `chatWithNIM` (or into a small NIM client struct) so tests and callers can configure endpoints without altering global state.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Code Review
This pull request successfully adds conversational context to the interactive CLI chat for both NIM and other models. It correctly maintains a history of messages and sends the full context with each request. The changes also include significant improvements, such as replacing manual JSON string construction and parsing with standard library functions (json.Marshal and json.Unmarshal), making the code more robust and secure. A new test has been added to verify the context-aware behavior for NIM chat, which is excellent. My review includes a few suggestions to improve code clarity, reduce redundancy, and fix a slightly misleading comment.
…nagement in chat functions
…add package declaration to nim.go.
ilopezluna
left a 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.
Thanks for working on this @nithish-95 !
I have tried your PR and it works well, nice 👏
Added a couple of minor suggestions, and a questions related to llamacppgo and download.go changes (I think we should not change these files).
Let me know what do you think 🙏
| @@ -28,7 +27,7 @@ const ( | |||
| var ( | |||
| ShouldUseGPUVariant bool | |||
| ShouldUseGPUVariantLock sync.Mutex | |||
| ShouldUpdateServer = true | |||
| ShouldUpdateServer = false | |||
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.
Why should we not update llama.cpp?
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.
Would be nice to have an explanation of why this needed to be changed alright. Maybe the PR is not ready for review?
| @@ -95,8 +95,8 @@ func (l *llamaCpp) Install(ctx context.Context, httpClient *http.Client) error { | |||
|
|
|||
| // We don't currently support this backend on Windows. We'll likely | |||
| // never support it on Intel Macs. | |||
| if (runtime.GOOS == "darwin" && runtime.GOARCH == "amd64") || | |||
| (runtime.GOOS == "windows" && runtime.GOARCH != "amd64" && runtime.GOARCH != "arm64") { | |||
| // We don't currently support this backend on Windows. | |||
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.
| // We don't currently support this backend on Windows. |
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.
Yes duplicate
| if (runtime.GOOS == "darwin" && runtime.GOARCH == "amd64") || | ||
| (runtime.GOOS == "windows" && runtime.GOARCH != "amd64" && runtime.GOARCH != "arm64") { | ||
| // We don't currently support this backend on Windows. | ||
| if (runtime.GOOS == "windows" && !(runtime.GOARCH == "amd64" || runtime.GOARCH == "arm64")) { |
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.
Is it Intel Mac supported? If so we should update the comment. If not we should get back the if (runtime.GOOS == "darwin" && runtime.GOARCH == "amd64") ||
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.
Personally I'd vote for removing lines 96 to 101 completely. But this is fine.
When somebody encounters an error from a new platform, I think this is good. It would be nice to encourage people to explore these errors and even enable new platforms (from a community perspective).
I think we try too hard to block code execution for things deemed unsupported. If the community wants to work on Intel Mac and get that working, so be it.
Now whether than propagates to Docker Desktop, is a different discussion (and unlikely since it's a legacy platform). We have the same discussion for Linux DMR.
| requestPayload := struct { | ||
| Model string `json:"model"` | ||
| Messages []desktop.OpenAIChatMessage `json:"messages"` | ||
| Stream bool `json:"stream"` | ||
| }{ | ||
| Model: modelName, | ||
| Messages: *messages, | ||
| Stream: 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.
| requestPayload := struct { | |
| Model string `json:"model"` | |
| Messages []desktop.OpenAIChatMessage `json:"messages"` | |
| Stream bool `json:"stream"` | |
| }{ | |
| Model: modelName, | |
| Messages: *messages, | |
| Stream: true, | |
| } | |
| requestPayload := desktop.OpenAIChatRequest{ | |
| Model: modelName, | |
| Messages: *messages, | |
| Stream: true, | |
| } |
ericcurtin
left a 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.
Could the NIM client and client for other backends be consolidated further? @nithish-95 poke us again when it is ready for review. Please test thoroughly (although I have faith you already are). Thanks for pushing this
|
I meant to press comment instead of approve :) |
Fixes #351
This PR addresses an issue where the interactive chat in the CLI was not sending the full conversational context with each message. This resulted in the model not being able to remember previous parts of the conversation.
The following changes have been made:
messagesslice is now maintained to store the history of the conversation.This ensures that the model has the full context of the conversation, as demonstrated in the updated behavior.