-
Notifications
You must be signed in to change notification settings - Fork 46
mcp: memory server example #90
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
Conversation
I'm not sure I understand the purpose of this example. There is a lot of code implementing an in-memory relational DB, right? But what's the relationship to MCP? How does it illustrate this SDK? |
examples/memory/kb.go
Outdated
data, err := os.ReadFile(fs.path) | ||
if err != nil { | ||
if os.IsNotExist(err) { | ||
return []byte{}, nil |
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.
return nil, nil?
examples/memory/kb.go
Outdated
return KnowledgeGraph{}, fmt.Errorf("failed to unmarshal from store: %w", err) | ||
} | ||
|
||
graph := KnowledgeGraph{ |
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.
KnowledgeGraph{}
examples/memory/kb.go
Outdated
|
||
for _, obs := range observations { | ||
entityIndex := -1 | ||
for i, entity := range graph.Entities { |
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.
use slices.IndexFunc.
examples/memory/kb.go
Outdated
var newEntities []Entity | ||
for _, entity := range entities { | ||
exists := false | ||
for _, existingEntity := range graph.Entities { |
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.
Use slices.ContainsFunc.
examples/memory/kb.go
Outdated
|
||
var newObservations []string | ||
for _, content := range obs.Contents { | ||
exists := slices.Contains(graph.Entities[entityIndex].Observations, content) |
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.
Inline the call into the if statement.
examples/memory/kb.go
Outdated
|
||
// Filter relations | ||
var filteredRelations []Relation | ||
for _, relation := range graph.Relations { |
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.
slices.DeleteFunc?
examples/memory/kb.go
Outdated
|
||
// Filter observations | ||
var filteredObservations []string | ||
for _, observation := range entity.Observations { |
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.
slices.DeleteFunc?
examples/memory/kb.go
Outdated
} | ||
|
||
var filteredRelations []Relation | ||
for _, existingRelation := range graph.Relations { |
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.
You can probably use one or more slices functions here too.
examples/memory/kb.go
Outdated
} | ||
|
||
// readGraph returns the complete knowledge graph. | ||
func (k knowledgeBase) readGraph() (KnowledgeGraph, error) { |
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.
Same as loadGraph—why have it?
Note: I've requested a lot of changes, but they may be moot if we don't have a compelling reason to include this example. |
This example is listed on the https://modelcontextprotocol.io/examples (mentioned on the first post of #33) as:
From the purpose point of view, it might be redundant to other examples, that is, to show how to serve Tools using this sdk. |
Thanks for the feedback, I've addressed it, in case we decide to keep this example in this repo. Also, in this commits, I've already using a new API for this example. |
I see now that it matches an example in the servers repo. I'm OK keeping it. We should find a way to highlight the MCP parts. I'll think about that. |
examples/memory/kb.go
Outdated
} | ||
|
||
// createEntities adds new entities to the graph, skipping duplicates by name. | ||
// Returns the new entities that were actually added. |
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.
"It returns"
examples/memory/kb.go
Outdated
|
||
graph, err := k.loadGraph() | ||
if err != nil { | ||
res.IsError = 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.
Just return the error normally, and the SDK will do this for you.
examples/memory/kb.go
Outdated
|
||
graph, err := k.searchNodes(params.Arguments.Query) | ||
if err != nil { | ||
res.IsError = 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.
ditto
examples/memory/kb_test.go
Outdated
// Confirm observation removal | ||
graph, _ = kb.loadGraph() | ||
aliceFound := false | ||
for _, e := range graph.Entities { |
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.
Can you simplify this with a function from the slices package?
examples/memory/kb.go
Outdated
|
||
observations, err := k.addObservations(params.Arguments.Observations) | ||
if err != nil { | ||
res.IsError = 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.
ditto
examples/memory/kb_test.go
Outdated
) | ||
|
||
// getStoreFactories provides test factories for both storage implementations. | ||
func getStoreFactories() map[string]func(t *testing.T) store { |
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.
call this function stores
examples/memory/kb_test.go
Outdated
func TestKnowledgeBaseOperations(t *testing.T) { | ||
factories := getStoreFactories() | ||
|
||
for name, factory := range factories { |
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.
for name, newStore := range stores
same below
examples/memory/main.go
Outdated
} | ||
|
||
// Entity represents a knowledge graph node with observations. | ||
type Entity struct { |
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.
This and the next three should be in kb.go.
examples/memory/kb_test.go
Outdated
t.Errorf("expected error content in MCP response") | ||
} else { | ||
// Verify the error message contains expected text | ||
expectedErrorMsg := "entity with name NonExistentEntity not found" |
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.
just want
is fine
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.
Thank you!
Part of #33 . This PR adds an example of implementing go-sdk on memory server.