-
Notifications
You must be signed in to change notification settings - Fork 85
[Server] Add support on Builder to inject Registry instance #146
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
|
Hi @chr-hertel I've been working on the Symfony profiler integration (the use case from #74), and I found a solution using the loader pattern that @soyuka suggested in PR #75. My approach: I created a ProfilingLoader that implements LoaderInterface to capture the Registry reference: final class ProfilingLoader implements LoaderInterface
{
private ?ReferenceRegistryInterface $registry = null;
public function load(ReferenceRegistryInterface $registry): void
{
// Capture the registry reference for profiling
$this->registry = $registry;
}
public function getRegistry(): ?ReferenceRegistryInterface
{
return $this->registry;
}
}Then register it in the Symfony container and add it to the ServerBuilder: ->set('ai.mcp.profiling_loader', ProfilingLoader::class)
->set('mcp.server.builder', ServerBuilder::class)
->factory([Server::class, 'builder'])
// ...
->call('addLoaders', [service('ai.mcp.profiling_loader')])The Symfony DataCollector then reads from this ProfilingLoader in lateCollect(): public function lateCollect(): void
{
$registry = $this->profilingLoader->getRegistry();
if (null === $registry) {
// Handle case where Registry wasn't built yet
return;
}
// Collect tools, prompts, resources, templates
foreach ($registry->getTools()->references as $tool) {
// ...
}
}This approach solves my use case. If you think your PR #146 is "not exactly ideal" and this loader approach (following the pattern from #111) suits you, maybe this PR becomes less necessary? I'll let you and @soyuka decide what's best 🙂 |
|
yea, i can see that working, you really dug for a solution & found one👍 i think this PR would enable us to manage the |
Would you recommend I wait for this PR to be merged before refactoring profiler to use |
soyuka
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.
My suggestion would be to allow server capabilities inside the constructor as well. Definitely +1 to allow to change the registry.
| { | ||
| $this->serverCapabilities = $serverCapabilities; | ||
| } | ||
|
|
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'd be even cleaner to have the ServerCapabilities directly in dependency injection.
return $this->serverCapabilities ?? new ServerCapabilities(
tools: [] !== $this->tools,
toolsListChanged: $this->eventDispatcher instanceof EventDispatcherInterface,
resources: [] !== $this->resources || [] !== $this->resourceTemplates,
resourcesSubscribe: false,
resourcesListChanged: $this->eventDispatcher instanceof EventDispatcherInterface,
prompts: [] !== $this->prompts,
promptsListChanged: $this->eventDispatcher instanceof EventDispatcherInterface,
logging: false,
completions: true,
);This would allow more flexibility in my opinion. Loaders could then prepare tools, resources etc. without having to register them at runtime. In the end, they could more easily be cache-able. I'd still keep register methods for runtime changes.
| public function getCapabilities(): ServerCapabilities; | ||
|
|
||
| public function setServerCapabilities(ServerCapabilities $serverCapabilities): void; |
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.
| public function getCapabilities(): ServerCapabilities; | |
| public function setServerCapabilities(ServerCapabilities $serverCapabilities): void; |
Not exactly ideal and doesn't solve the trouble i have with that service slicing at this point, but would enable the concerns of #74, doesn't it? @camilleislasse
cc @soyuka