Conversation
| return fmt.Errorf("invalid embeddings dimensions: %d", cfg.Embeddings.Dimensions) | ||
| } | ||
|
|
||
| db, err := database.NewPostgreSQL(ctx, cfg.DatabaseURL) |
There was a problem hiding this comment.
I think this logic should be on the server instead (registry service) and the CLI should just call the functionality - this will allow us to manage the functionality through the UI too
There was a problem hiding this comment.
100%, that way this can also be managed from the UI
There was a problem hiding this comment.
In face, the CLI code should be moved to a folder where it cannot import any DB logic so this cannot happen again
|
|
||
| processServers := embeddingsIncludeServers | ||
| processAgents := embeddingsIncludeAgents | ||
| if !processServers && !processAgents { |
There was a problem hiding this comment.
can this happen? we default to "true" for both of those flags
| services: | ||
| postgres: | ||
| image: postgres:16-alpine | ||
| image: pgvector/pgvector:pg16 |
There was a problem hiding this comment.
I am assuming this is the same as postgre,s but with the vector stuff added?
| // ServerResponseMeta mirrors the MCP ResponseMeta but adds semantic metadata. | ||
| type ServerResponseMeta struct { | ||
| Official *apiv0.RegistryExtensions `json:"io.modelcontextprotocol.registry/official,omitempty"` | ||
| Semantic *ServerSemanticMeta `json:"agentregistry.solo.io/semantic,omitempty"` | ||
| } |
There was a problem hiding this comment.
I believe this change means that our server is no longer compliant with the spec, is that something we care about? @pmuir
There was a problem hiding this comment.
no, we should still be complaint with the spec. the published api will look like:
{
"server": { ... publisher metadata only ... },
"_meta": {
"io.modelcontextprotocol.registry/official": {...},
"agentregistry.solo.io/semantic": {"score": ...}
}
}where the old api would be:
{
"server": { ... publisher metadata only ... },
"_meta": {
"io.modelcontextprotocol.registry/official": {...}
}
}i don't beleive the api spec has constraints on the content of the _meta field unless i am mistaken?
| // Serialize template creation/migration across concurrent test processes to | ||
| // avoid racing on extension creation (which can violate pg_extension_name_index). | ||
| // Use a global advisory lock key to coordinate. | ||
| const lockKey int64 = 0x61726567 // "areg" prefix |
There was a problem hiding this comment.
Why did this become necessary, this seems pretty hacky
There was a problem hiding this comment.
this is necessary because we need to initialize the pg vector extension in our testbed which can lead to races. this is a standard way of doing a global db lock which prevents race conditions (which we were hitting in CI before adding this lock). can you explain what's hacky about it?
| } | ||
| defer func() { _ = conn.Close(ctx) }() | ||
|
|
||
| if _, err := conn.Exec(ctx, "CREATE EXTENSION IF NOT EXISTS vector"); err != nil { |
There was a problem hiding this comment.
Isn't this done by one of the migrations?
There was a problem hiding this comment.
the tests do not run the migrations on every run (would be slow & prevent parallelization). so we simply clone the db template and run this query on each test, which is why it is needed
| } | ||
| } | ||
|
|
||
| func (s *registryServiceImpl) prepareSemanticOptions(ctx context.Context, filter interface{}) error { |
There was a problem hiding this comment.
This seems like a weird helper function if 90% of the code isn't shared, also checking filter==nil will never work because the value being passed in isn't an interface{} but a real type.
There was a problem hiding this comment.
the purpose of the helper is to avoid duplicating the same ensureSemanticEmbedding call for both ServerFilter and AgentFilter. so i'm not sure what's weird about it. can you explain?
but yes i will go ahead and drop the outer filter==nil check
There was a problem hiding this comment.
But that's what I'm saying, the caller could just call s.ensureSemanticEmbedding(ctx, f.Semantic)? Or is the purpose just the nil check? Maybe we should create an interface for this type? I Don't feel super strongly about this, but in general I shy away from any/interface{} types in code, plus this helper function basically just checks nil. Maybe we should put this logic on database.ServerFilter or something
There was a problem hiding this comment.
im fine either way. removed the code as you requested
3 participants
No description provided.