-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
feat: much improved RAG, added LLM post-processing of results #435
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 318099c in 2 minutes and 3 seconds
More details
- Looked at
185
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. gptme/config.py:91
- Draft comment:
Consider clarifying the union type usage in isinstance(self.rag, dict | Container). - Reason this comment was not posted:
Confidence changes required:33%
None
2. gptme/config.py:91
- Draft comment:
Use a tuple in the isinstance check to avoid runtime errors; change 'dict | Container' to '(dict, Container)'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment appears to be incorrect. The|
syntax for union types was introduced in Python 3.10 and works in isinstance checks. The code base appears to be using modern Python features (given the presence of type hints and union types elsewhere). There's no strong evidence that this would cause runtime errors.
I could be wrong if this codebase needs to support older Python versions. The tuple syntax would work in all Python versions.
Even if older Python versions need to be supported, this should be caught by the build/CI system running tests on the supported Python versions. We don't need a comment for this.
The comment should be deleted as it suggests changing valid modern Python syntax without clear evidence of a problem, and version compatibility issues would be caught by the build system.
3. gptme/config.py:43
- Draft comment:
Insert a missing space between 'relevant' and 'context' in the prompt string. - Reason this comment was not posted:
Marked as duplicate.
4. gptme/tools/rag.py:113
- Draft comment:
Split CLI flags into separate arguments using cmd.extend rather than appending a single concatenated string. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_Pqbj1snQtE3EQeqT
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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #435 +/- ##
===========================================
- Coverage 67.20% 56.87% -10.34%
===========================================
Files 70 70
Lines 6187 6226 +39
===========================================
- Hits 4158 3541 -617
- Misses 2029 2685 +656
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
1f02733
to
0b528aa
Compare
Does the following:
Important
Introduces
RagConfig
for RAG settings, adds post-processing prompt, and updates RAG tool for LLM post-processing support.RagConfig
class ingptme/config.py
for RAG settings.default_post_process_prompt
for context post-processing.ProjectConfig
to useRagConfig
.rag_enhance_messages
inrag.py
to support post-processing with LLM usingpost_process_model
andpost_process_prompt
.rag_search
to use--format full --print-relevance
.rag_enhance_messages
.This description was created by
for 318099c. It will automatically update as commits are pushed.