Skip to content
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

fix: LocalArchivalMemory prints ref_doc_info on if not using EmptyIndex #240

Merged

Conversation

goetzrobin
Copy link
Contributor

Currently, if you run the /memory command the application breaks if the LocalArchivalMemory has no existing archival storage and defaults to the EmptyIndex. This is caused by EmptyIndex not having a ref_doc_info implementation and throwing an Exception when that is used to print the memory information to the console. This hot fix simply makes sure that we do not try to use the function if using EmptyIndex and instead prints a message to the console indicating an EmptyIndex is used.

This is just a hot fix and I feel like there's definitely ways to improve this, but this might be "good enough" for now to run the /memory command successfully again.

I am pretty new to the python game so please feel free to adjust this PR as needed.

Happy to learn more about LLamaIndex if you guys have any recommended resources for me to check out that would be awesome!

@sarahwooders sarahwooders self-requested a review November 1, 2023 16:52
Copy link
Collaborator

@sarahwooders sarahwooders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great catch, thanks!

It might be cleaner to mimic what's being done here https://github.com/cpacker/MemGPT/blob/main/memgpt/memory.py#L223, and return a string (rather than print) for both the empty/non-empty index cases. I think the empty string return was a temporary hack that I forgot to fix because I wasn't sure how to convert the VectorIndex into a nice string representation.

@goetzrobin
Copy link
Contributor Author

goetzrobin commented Nov 1, 2023

Agreed, @sarahwooders! That solution is much cleaner.

I saw your work on this: #226.

Happy to open a PR to that branch instead.

Let me know what you think! And thanks for all your great work on this!

It's cool to see all of your and your teams changes to this project! So freaking fast! It's awesome!

@sarahwooders
Copy link
Collaborator

Thanks! #226 is still a work in progress and we need this fix ASAP, so it would be better to merge this with main.

@goetzrobin goetzrobin force-pushed the fix-ref_doc_info-error-empty-index branch from 082ddb1 to bc2ba98 Compare November 2, 2023 00:15
@goetzrobin
Copy link
Contributor Author

@sarahwooders pushed a new version based on your suggestion. let me know if this looks good! Thank you!

Currently, if you run the /memory command the application breaks if the LocalArchivalMemory
has no existing archival storage and defaults to the EmptyIndex. This is caused by EmptyIndex
not having a ref_doc_info implementation and throwing an Exception when that is used to print
the memory information to the console. This hot fix simply makes sure that we do not try to
use the function if using EmptyIndex and instead prints a message to the console indicating
an EmptyIndex is used.
Copy link
Collaborator

@sarahwooders sarahwooders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! Congrats on your first contribution :)

@sarahwooders sarahwooders merged commit 54e5aef into letta-ai:main Nov 2, 2023
mattzh72 pushed a commit that referenced this pull request Oct 9, 2024
…ex (#240)

Currently, if you run the /memory command the application breaks if the LocalArchivalMemory
has no existing archival storage and defaults to the EmptyIndex. This is caused by EmptyIndex
not having a ref_doc_info implementation and throwing an Exception when that is used to print
the memory information to the console. This hot fix simply makes sure that we do not try to
use the function if using EmptyIndex and instead prints a message to the console indicating
an EmptyIndex is used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants