-
-
Notifications
You must be signed in to change notification settings - Fork 172
[AI Bundle] add ResetInterface to prevent memory leaks
#1533
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?
[AI Bundle] add ResetInterface to prevent memory leaks
#1533
Conversation
|
Status: needs work I do not finish the manual tests yet |
266afd6 to
ade5b26
Compare
ade5b26 to
aa97ee1
Compare
aa97ee1 to
a3d001b
Compare
ResetInterface to prevent memory leaks
a3d001b to
6c7526e
Compare
ResetInterface to prevent memory leaksResetInterface to prevent memory leaks
|
Thanks for the approval! ❤️ I'll continue investigating. Edit: I found and pushed a solution, but I'm not sure if it's correct diff --git a/src/ai-bundle/src/Profiler/TraceableChat.php b/src/ai-bundle/src/Profiler/TraceableChat.php
index 6755669d..df838b31 100644
--- a/src/ai-bundle/src/Profiler/TraceableChat.php
+++ b/src/ai-bundle/src/Profiler/TraceableChat.php
@@ -70,6 +70,9 @@ final class TraceableChat implements ChatInterface, ResetInterface
public function reset(): void
{
+ if ($this->chat instanceof ResetInterface) {
+ $this->chat->reset();
+ }
$this->calls = [];
}
}
diff --git a/src/ai-bundle/src/Profiler/TraceableMessageStore.php b/src/ai-bundle/src/Profiler/TraceableMessageStore.php
index d8eb416e..077f7ef6 100644
--- a/src/ai-bundle/src/Profiler/TraceableMessageStore.php
+++ b/src/ai-bundle/src/Profiler/TraceableMessageStore.php
@@ -73,6 +73,9 @@ final class TraceableMessageStore implements ManagedStoreInterface, MessageStore
public function reset(): void
{
+ if ($this->messageStore instanceof ResetInterface) {
+ $this->messageStore->reset();
+ }
$this->calls = [];
}
} |
6c7526e to
aa6ac29
Compare
|
I think another alternative could be to delete the proxies and set lazy to false in these two services: WDYT? Edit: |
|
Friendly Ping @Guikingone |
|
Hi @santysisi 👋🏻 Good for me to remove |
25a63c9 to
627557c
Compare
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.
Pull request overview
This PR addresses memory leak issues in FrankenPHP worker mode and Messenger consumption by implementing ResetInterface on traceable services and in-memory stores. The changes ensure that profiling data and in-memory storage are properly cleared between requests.
Changes:
- Added
ResetInterfacesupport to in-memory stores (Chat and Store components) to clear stored data between requests - Added
ResetInterfacesupport to all traceable profiler decorators (Platform, Chat, MessageStore, Toolbox, and Registry) to clear profiling data - Removed lazy loading from in-memory stores since
ResetInterfacerequires working with actual instances rather than proxies - Added
symfony/service-contractsdependency to affected packages forResetInterface
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/store/src/InMemory/Store.php | Implements ResetInterface with reset() delegating to drop() |
| src/store/tests/InMemory/StoreTest.php | Adds test verifying reset clears documents |
| src/store/composer.json | Adds symfony/service-contracts dependency |
| src/store/CHANGELOG.md | Documents ResetInterface addition |
| src/chat/src/InMemory/Store.php | Implements ResetInterface with reset() clearing all messages |
| src/chat/tests/InMemory/StoreTest.php | Adds test verifying reset clears messages |
| src/chat/composer.json | Adds symfony/service-contracts dependency |
| src/chat/CHANGELOG.md | Documents ResetInterface addition |
| src/mcp-bundle/src/Profiler/TraceableRegistry.php | Implements ResetInterface delegating to clear() |
| src/mcp-bundle/tests/Profiler/TraceableRegistryTest.php | Adds test verifying reset calls clear() |
| src/mcp-bundle/src/McpBundle.php | Adds kernel.reset tag to traceable registry service |
| src/mcp-bundle/composer.json | Adds symfony/service-contracts dependency |
| src/mcp-bundle/CHANGELOG.md | Documents ResetInterface addition |
| src/ai-bundle/src/Profiler/TraceableToolbox.php | Implements ResetInterface to clear calls array |
| src/ai-bundle/src/Profiler/TraceablePlatform.php | Implements ResetInterface to clear calls and resultCache |
| src/ai-bundle/src/Profiler/TraceableMessageStore.php | Implements ResetInterface with delegation to wrapped store |
| src/ai-bundle/src/Profiler/TraceableChat.php | Implements ResetInterface with delegation to wrapped chat |
| src/ai-bundle/tests/Profiler/TraceableToolboxTest.php | Adds test verifying reset clears calls |
| src/ai-bundle/tests/Profiler/TraceablePlatformTest.php | Adds test verifying reset clears calls and resultCache |
| src/ai-bundle/tests/Profiler/TraceableMessageStoreTest.php | Adds test verifying reset clears calls |
| src/ai-bundle/tests/Profiler/TraceableChatTest.php | Adds test verifying reset clears calls |
| src/ai-bundle/src/AiBundle.php | Adds kernel.reset tags to all traceable services and in-memory stores; removes lazy loading and proxy tags from in-memory stores |
| src/ai-bundle/tests/DependencyInjection/AiBundleTest.php | Updates tests to expect non-lazy in-memory stores with kernel.reset tag instead of proxy tags |
| src/ai-bundle/composer.json | Adds symfony/service-contracts dependency |
| src/ai-bundle/CHANGELOG.md | Documents ResetInterface addition to traceable services |
627557c to
0adcdfb
Compare
0adcdfb to
88ecda9
Compare
|
rebase unlocked |
|
after a rebase, we can merge this PR |
|
Hi, I don't have my computer right now 😔 |
This PR introduces the
ResetInterfaceandresettags to the tracers and in memory stores. These changes help optimize memory usage by preventing unnecessary memory increases with each request. Additionally, they ensure that erroneous information is no longer displayed in the profiler, improving accuracy and performance monitoring.