fix: assign current_time to datetime.now() if current_time is None #5045

mbchang · 2023-05-20T20:14:51Z

Assign `current_time` to `datetime.now()` if it `current_time is None` in `time_weighted_retriever`

As implemented, add_documents in TimeWeightedVectorStoreRetriever assigns doc.metadata["last_accessed_at"] and doc.metadata["created_at"] to datetime.datetime.now() if current_time is not in kwargs.

    def add_documents(self, documents: List[Document], **kwargs: Any) -> List[str]:
        """Add documents to vectorstore."""
        current_time = kwargs.get("current_time", datetime.datetime.now())
        # Avoid mutating input documents
        dup_docs = [deepcopy(d) for d in documents]
        for i, doc in enumerate(dup_docs):
            if "last_accessed_at" not in doc.metadata:
                doc.metadata["last_accessed_at"] = current_time
            if "created_at" not in doc.metadata:
                doc.metadata["created_at"] = current_time
            doc.metadata["buffer_idx"] = len(self.memory_stream) + i
        self.memory_stream.extend(dup_docs)
        return self.vectorstore.add_documents(dup_docs, **kwargs)

However, from the way add_documents is being called from GenerativeAgentMemory, current_time is set as a kwarg, but it is given a value of None:

    def add_memory(
        self, memory_content: str, now: Optional[datetime] = None
    ) -> List[str]:
        """Add an observation or memory to the agent's memory."""
        importance_score = self._score_memory_importance(memory_content)
        self.aggregate_importance += importance_score
        document = Document(
            page_content=memory_content, metadata={"importance": importance_score}
        )
        result = self.memory_retriever.add_documents([document], current_time=now)

The default of now was set in #4658 to be None. The proposed fix is the following:

    def add_documents(self, documents: List[Document], **kwargs: Any) -> List[str]:
        """Add documents to vectorstore."""
        current_time = kwargs.get("current_time", datetime.datetime.now())
        # `current_time` may exist in kwargs, but may still have the value of None.
        if current_time is None:
            current_time = datetime.datetime.now()

Alternatively, we could just set the default of now to be datetime.datetime.now() everywhere instead. Thoughts @hwchase17? If we still want to keep the default to be None, then this PR should fix the above issue. If we want to set the default to be datetime.datetime.now() instead, I can update this PR with that alternative fix. EDIT: seems like from #5018 it looks like we would prefer to keep the default to be None, in which case this PR should fix the error.

Before submitting

Who can review?

Community members can review the PR once tests pass. Tag maintainers/contributors who might be interested:

@hwchase17
@dev2049

hwchase17

if doing this, can just do current_time = kwargs.get("current_time") in line above

mbchang · 2023-05-21T04:57:50Z

current_time = kwargs.get("current_time") is not sufficient, because even though the variable current_time exists as a kwarg, the value passed for the current_time is None. This causes doc.metadata["last_accessed_at"] = None and doc.metadata["created_at"] = None, which causes issue #4825

hwchase17 · 2023-05-21T05:08:34Z

current_time = kwargs.get("current_time") is not sufficient, because even though the variable current_time exists as a kwarg, the value passed for the current_time is None. This causes doc.metadata["last_accessed_at"] = None and doc.metadata["created_at"] = None, which causes issue #4825

i mean keep the added lines, but just remove the default of datatime.now - since you check for None after, it doesnt do anything

…rrent_time')

mbchang · 2023-05-22T22:25:25Z

oh yeah makes sense; I've updated it

@dev2049

…ever (#5155) # Same as PR #5045, but for async   Fixes #4825 I had forgotten to update the asynchronous counterpart `aadd_documents` with the bug fix from PR #5045, so this PR also fixes `aadd_documents` too. ## Who can review? Community members can review the PR once tests pass. Tag maintainers/contributors who might be interested: @dev2049

@dev2049

…ever (#5155) # Same as PR #5045, but for async   Fixes #4825 I had forgotten to update the asynchronous counterpart `aadd_documents` with the bug fix from PR #5045, so this PR also fixes `aadd_documents` too. ## Who can review? Community members can review the PR once tests pass. Tag maintainers/contributors who might be interested: @dev2049

fix: assign current_time to datetime.now() if current_time is None

002b4e5

hwchase17 reviewed May 21, 2023

View reviewed changes

dev2049 mentioned this pull request May 22, 2023

Fix: Update memory.py #5018

Closed

style: remove datetime.datetime() as default value for kwargs.get('cu…

1eeda10

…rrent_time')

dev2049 merged commit e173e03 into langchain-ai:master May 22, 2023

mbchang mentioned this pull request May 23, 2023

fix: fix current_time=Now bug for aadd_documents in TimeWeightedRetriever #5155

Merged

danielchalef mentioned this pull request Jun 5, 2023

Zep Hybrid Search #5742

Merged

This was referenced Jun 25, 2023

Zep Authentication #6725

Closed

Zep Authentication #6728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: assign current_time to datetime.now() if current_time is None #5045

fix: assign current_time to datetime.now() if current_time is None #5045

mbchang commented May 20, 2023 •

edited

Loading

hwchase17 left a comment

mbchang commented May 21, 2023

hwchase17 commented May 21, 2023

mbchang commented May 22, 2023

fix: assign current_time to datetime.now() if current_time is None #5045

fix: assign current_time to datetime.now() if current_time is None #5045

Conversation

mbchang commented May 20, 2023 • edited Loading

Assign current_time to datetime.now() if it current_time is None in time_weighted_retriever

Before submitting

Who can review?

hwchase17 left a comment

Choose a reason for hiding this comment

mbchang commented May 21, 2023

hwchase17 commented May 21, 2023

mbchang commented May 22, 2023

mbchang commented May 20, 2023 •

edited

Loading

Assign `current_time` to `datetime.now()` if it `current_time is None` in `time_weighted_retriever`