-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
refactor(rag): improve RAG manager implementation #268
Conversation
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.
❌ Changes requested. Reviewed everything up to 6d52f30 in 1 minute and 41 seconds
More details
- Looked at
137
lines of code in3
files - Skipped
1
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_p2M0n9iq439lwJmQ
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -20,10 +23,21 @@ | |||
_HAS_RAG = False | |||
|
|||
|
|||
# Shared RAG manager instance | |||
_rag_manager: "RAGManager | None" = None |
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.
Using a global variable for _rag_manager
can lead to issues in multi-threaded environments or when the state needs to be reset. Consider using a class or context manager to handle the RAG manager instance.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #268 +/- ##
==========================================
+ Coverage 72.36% 72.52% +0.15%
==========================================
Files 64 64
Lines 4313 4316 +3
==========================================
+ Hits 3121 3130 +9
+ Misses 1192 1186 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
❌ Changes requested. Incremental review on 46cf058 in 38 seconds
More details
- Looked at
46
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_EcIhhVh9TNumyMlF
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Makefile
Outdated
@@ -146,7 +151,7 @@ cloc-server: | |||
cloc gptme/server --by-file | |||
|
|||
cloc-tests: | |||
cloc tests/*.py --by-file | |||
cloc tests --by-file |
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.
Changing cloc tests/*.py
to cloc tests
might include non-Python files in the count, leading to inaccurate LoC statistics. Consider reverting to cloc tests/*.py
.
cloc tests --by-file | |
cloc tests/*.py --by-file |
- Introduce shared RAG manager singleton via get_rag_manager() - Add DEFAULT_COLLECTION constant for consistent collection naming - Remove unused _clear_cache function - Update gptme-rag dependency to 0.3.1
46cf058
to
0e9cb11
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.
👍 Looks good to me! Incremental review on 0e9cb11 in 51 seconds
More details
- Looked at
137
lines of code in3
files - Skipped
1
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_sJAu7mC5IPqELk8I
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
- Fix type error by using correct internal path to _rag_manager - Split imports for better readability - Organize imports more clearly
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.
👍 Looks good to me! Incremental review on edbf520 in 24 seconds
More details
- Looked at
33
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. tests/test_tools_rag.py:10
- Draft comment:
Combine multiple imports from the same module into a single line for better readability.
from gptme.tools.rag import _HAS_RAG, init as init_rag, rag_index, rag_search
- Reason this comment was not posted:
Confidence changes required:10%
The import statements can be optimized for readability and maintainability.
Workflow ID: wflow_F4KAVc4T7bEKKLFC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Refactor RAG manager to use a shared singleton, add a default collection constant, remove an unused function, and update the
gptme-rag
dependency.RAGManager
viaget_rag_manager()
in_rag_context.py
.rag_manager
initialization inrag.py
withget_rag_manager()
calls.DEFAULT_COLLECTION
in_rag_context.py
for consistent collection naming._clear_cache()
function in_rag_context.py
.gptme-rag
dependency to version0.3.1
inpyproject.toml
.This description was created by for edbf520. It will automatically update as commits are pushed.