feat: support seperate memory - #3
Conversation
|
Here's the code health analysis summary for commits Analysis Summary
DeepSource Report Card: B
Focus area: Reliability — Fix the double application of prefixes corrupting the prompt history format in `api/core/memory/node_scoped_memory.py`.
|
![deepsource-development[bot]](https://avatars.githubusercontent.com/u/40287229?s=60&v=4){kind=small}
| @@ -8,6 +8,7 @@ | |||
| from core.app.entities.app_invoke_entities import ModelConfigWithCredentialsEntity | |||
| from core.entities.provider_entities import QuotaUnit | |||
| from core.file.models import File | |||
There was a problem hiding this comment.
Unused File import increases code clutter
The File object imported from core.file.models is not used anywhere in the module, which adds unnecessary code clutter and can confuse maintainers. This can also slightly affect code readability and static analysis.
Remove the unused File import statement to clean up and simplify the codebase.
| ) -> str: ... | ||
|
|
||
|
|
||
| class LLMNode(Node): |
There was a problem hiding this comment.
Unimplemented @abstractmethod risks incomplete class behavior
The class LLMNode inherits from Node but does not override all methods marked with @abstractmethod in the base class. This omission can lead to runtime errors or unintended behavior when abstract contracts are not fulfilled.
Implement all abstract methods defined in Node in the LLMNode class to ensure complete and correct behavior.
| self.credentials = {} | ||
|
|
||
| class _FakeManager: | ||
| def get_model_instance(self, *args, **kwargs): |
There was a problem hiding this comment.
Method without self usage wastes instance memory
The method get_model_instance does not use the self parameter, so it does not depend on the instance state. Binding this method as an instance method creates unnecessary overhead for each class instance.
Add the @staticmethod decorator before the method definition to avoid binding it to instances, conserving memory and reducing call overhead.
| monkeypatch.setattr(LLMNode, "_run", _fake_run) | ||
|
|
||
| # Run node | ||
| events = list(llm_node._run()) |
There was a problem hiding this comment.
Unused variable events occupies memory unnecessarily
The variable events is assigned the list from llm_node._run() but never used, leading to unnecessary memory consumption and potential confusion for maintainers. This appears on line 267.
Remove the assignment of events if the result is not needed, or rename it to _ or start with _unused to explicitly mark it as intentionally unused.
| # Create a fake invoke_llm that just produces a completion event | ||
| def _fake_invoke_llm_simple(self, **kwargs): | ||
| from core.model_runtime.entities.llm_entities import LLMUsage | ||
| from core.workflow.node_events.node import ModelInvokeCompletedEvent |
There was a problem hiding this comment.
Repeated import of ModelInvokeCompletedEvent causes confusion
The ModelInvokeCompletedEvent is imported multiple times within the same file or context. This duplication can confuse maintainers and lead to inconsistent usage or accidental shadowing.
Remove redundant imports of ModelInvokeCompletedEvent and consolidate imports to a single statement to maintain code clarity and reduce potential bugs.
| prefix = human_prefix if role_name == PromptMessageRole.USER else ai_prefix | ||
| messages.append( | ||
| UserPromptMessage(content=f"{prefix}: {it.text}") | ||
| if role_name == PromptMessageRole.USER | ||
| else AssistantPromptMessage(content=f"{prefix}: {it.text}") | ||
| ) | ||
|
|
||
| if messages: | ||
| tokens = self.model_instance.get_llm_num_tokens(messages) | ||
| while tokens > max_token_limit and len(messages) > 1: | ||
| messages.pop(0) | ||
| tokens = self.model_instance.get_llm_num_tokens(messages) | ||
|
|
||
| # Convert back to the required text format | ||
| lines: list[str] = [] | ||
| for m in messages: | ||
| if m.role == PromptMessageRole.USER: | ||
| prefix = human_prefix | ||
| elif m.role == PromptMessageRole.ASSISTANT: | ||
| prefix = ai_prefix | ||
| else: | ||
| continue | ||
| if isinstance(m.content, list): | ||
| # Only text content was saved in this minimal implementation | ||
| texts = [c.data for c in m.content if isinstance(c, TextPromptMessageContent)] | ||
| text = "\n".join(texts) | ||
| else: | ||
| text = str(m.content) | ||
| lines.append(f"{prefix}: {text}") |
There was a problem hiding this comment.
Prefixes are applied twice to history text
The method get_history_prompt_text incorrectly applies the human_prefix and ai_prefix to the message content twice. The prefix is first added when creating PromptMessage objects for token counting, and then it is added again when formatting the final string output, resulting in erroneous output like Human: Human: some user text.
Remove the second prefix application from the final loop that generates the text lines. The message content, which is converted to text, already contains the necessary prefix.
| if independent_scope and node_memory: | ||
| node_memory.clear() |
There was a problem hiding this comment.
Node-scoped memory is cleared regardless of the clear_after_execution setting
The code checks if independent_scope is enabled and clears node_memory if it exists. However, it fails to check the clear_after_execution flag from the node's memory configuration, causing data loss for users who expect memory to persist.
Add a condition to check self._node_data.memory.clear_after_execution before calling node_memory.clear().
| UserPromptMessage(content=f"{prefix}: {it.text}") | ||
| if role_name == PromptMessageRole.USER | ||
| else AssistantPromptMessage(content=f"{prefix}: {it.text}") | ||
| ) | ||
|
|
||
| if messages: | ||
| tokens = self.model_instance.get_llm_num_tokens(messages) | ||
| while tokens > max_token_limit and len(messages) > 1: | ||
| messages.pop(0) | ||
| tokens = self.model_instance.get_llm_num_tokens(messages) | ||
|
|
||
| # Convert back to the required text format | ||
| lines: list[str] = [] | ||
| for m in messages: | ||
| if m.role == PromptMessageRole.USER: | ||
| prefix = human_prefix | ||
| elif m.role == PromptMessageRole.ASSISTANT: | ||
| prefix = ai_prefix | ||
| else: | ||
| continue | ||
| if isinstance(m.content, list): | ||
| # Only text content was saved in this minimal implementation | ||
| texts = [c.data for c in m.content if isinstance(c, TextPromptMessageContent)] | ||
| text = "\n".join(texts) | ||
| else: | ||
| text = str(m.content) | ||
| lines.append(f"{prefix}: {text}") |
There was a problem hiding this comment.
Prefixes are applied twice, corrupting prompt history format
The get_history_prompt_text method incorrectly applies prefixes to history items twice. It first adds a prefix when creating PromptMessage objects, and then adds the same prefix again when converting these messages back to a text format, leading to corrupted output like Human: Human: ....
Remove the second prefixing step. The content of the PromptMessage object already contains the required prefix and text.
2 participants
No description provided.