feat(agents-api): Added mmr search and get history system tool + configurable doc search params in chat.py #940
Conversation
![standard-input[bot]](https://avatars.githubusercontent.com/in/1011034?s=60&v=4){kind=small}
Vedantsahai18
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 79ab127 in 1 minute and 23 seconds
More details
- Looked at
617lines of code in13files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/activities/execute_system.py:6
- Draft comment:
The removal of theHTTPExceptionimport might cause issues if it's used elsewhere in the file. Please ensure it's not needed before removing. - Reason this comment was not posted:
Confidence changes required:50%
The removal of the import statement forHTTPExceptioninexecute_system.pyseems unnecessary unless it's confirmed thatHTTPExceptionis not used anywhere in the file. This could lead to runtime errors ifHTTPExceptionis used but not imported.
2. agents-api/agents_api/workflows/task_execution/transition.py:17
- Draft comment:
The removal oftemporal_activity_after_retry_timeoutimport might cause issues if it's used elsewhere in the file. Please ensure it's not needed before removing. - Reason this comment was not posted:
Confidence changes required:50%
The removal oftemporal_activity_after_retry_timeoutimport intransition.pyshould be verified to ensure it's not used elsewhere in the file. This could lead to runtime errors if it's used but not imported.
3. agents-api/tests/test_execution_workflow.py:17
- Draft comment:
The removal ofasync_cozo_clientimport might cause issues if it's used elsewhere in the file. Please ensure it's not needed before removing. - Reason this comment was not posted:
Confidence changes required:50%
The removal ofasync_cozo_clientimport intest_execution_workflow.pyshould be verified to ensure it's not used elsewhere in the file. This could lead to runtime errors if it's used but not imported.
Workflow ID: wflow_sDQMpi5ThgRD1FVQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Vedantsahai18
force-pushed
the
f/chat-mmr
branch
from
79ab127 to
8f38603
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8f38603 in 19 seconds
More details
- Looked at
617lines of code in13files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. agents-api/agents_api/models/chat/gather_messages.py:30
- Draft comment:
The use ofmatchrequires Python 3.10 or later. Ensure the project supports this version or handle this with if-elif statements for compatibility with older versions. - Reason this comment was not posted:
Confidence changes required:50%
Theget_search_fn_and_paramsfunction uses a match-case statement, which is only available in Python 3.10 and later. This could cause compatibility issues if the project supports older Python versions.
2. agents-api/agents_api/models/chat/gather_messages.py:147
- Draft comment:
Ensurequery_embeddingis notNonebefore using it, especially whenrecall_options.modeistext. This could lead to issues if not handled properly. - Reason this comment was not posted:
Confidence changes required:50%
Thegather_messagesfunction usesquery_embeddingwithout checking if it isNone. This could lead to issues ifrecall_options.modeis set totext, wherequery_embeddingis not set.
Workflow ID: wflow_O0AjUzsUlOZbMoQc
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e2ea7f3 in 21 seconds
More details
- Looked at
36lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. agents-api/agents_api/activities/utils.py:311
- Draft comment:
Unused importget_history_query. Consider removing it to clean up the code. - Reason this comment was not posted:
Confidence changes required:50%
The import ofget_historyinutils.pyis not used anywhere in the file. This is unnecessary and should be removed to keep the code clean.
Workflow ID: wflow_r915ItKBShuYc4Td
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
@ellipsis-dev review this |
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 5ce27f5 in 1 minute and 27 seconds
More details
- Looked at
633lines of code in13files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. agents-api/agents_api/activities/execute_system.py:6
- Draft comment:
TheHTTPExceptionimport was removed, but it is still used in the file. This will cause a NameError. Please re-add the import. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. agents-api/agents_api/models/chat/gather_messages.py:153
- Draft comment:
Ensuresearch_fnis notNonebefore calling it to prevent a potential TypeError. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_ulhXqtedQJgO4LN6
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.
|
@ellipsis-dev generate a summary for this PR |
Pull Request SummaryThis pull request introduces several key updates:
|
Vedantsahai18
changed the title
Vedantsahai18
changed the title
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 11a98f9 in 92 minutes and 20 seconds
More details
- Looked at
470lines of code in6files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. memory-store/Dockerfile:20
- Draft comment:
Remove the commented-out RUN command if it's no longer needed:
# RUN cargo install --git https://github.com/cozo-community/cozo.git --branch f/publish-crate --rev 592f49b --profile release -F graph-algo -F jemalloc -F io-uring -F storage-new-rocksdb -F malloc-usable-size --target x86_64-unknown-linux-gnu cozo-ce-bin
- Reason this comment was not posted:
Confidence changes required:50%
In the Dockerfile, the commented-out RUN command should be removed if it's no longer needed. Additionally, the Dockerfile should specify the version of the base image to ensure reproducibility.
2. scripts/generate_changelog.py:174
- Draft comment:
Wrapos.remove('pr_data.json')in a try-except block to handle potential file not found errors:
try:
os.remove('pr_data.json')
logging.info("Deleted pr_data.json file")
except FileNotFoundError:
logging.warning("pr_data.json file not found")
- Reason this comment was not posted:
Confidence changes required:50%
In the Python script, the use ofos.remove('pr_data.json')should be wrapped in a try-except block to handle potential file not found errors gracefully.
Workflow ID: wflow_YAReB9rKwXX3bN8v
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.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5829b6e in 14 seconds
More details
- Looked at
11lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_QO6FPjZSZIl3kpyi
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 84a1a95 in 9 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/generate-changelog.yml:1
- Draft comment:
The PR description mentions removing unused imports and variables, but this is not reflected in the diff provided. Please ensure that all changes are included in the PR. - Reason this comment was not posted:
Confidence changes required:50%
The PR description mentions removing unused imports and variables, but this is not reflected in the diff provided. I will need to explore the codebase to verify this claim.
Workflow ID: wflow_sZRDG4gvedxEiTWg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d43cc8b in 10 seconds
More details
- Looked at
68lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/generate-changelog.yml:2
- Draft comment:
Consider re-adding therun-namekey for better logging and tracking of workflow runs. - Reason this comment was not posted:
Confidence changes required:33%
Therun-namekey was removed, but it might be useful for logging and tracking purposes.
Workflow ID: wflow_ft73rBGvygzhVeGy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Adds MMR search with configurable parameters, updates documentation, and introduces changelog generation workflow.
gather_messages.pywith configurablemmr_strength.get_search_fn_and_params()to select search function based onRecallOptions.limit,lang,metadata_filter,alpha,confidence.BaseDocSearchRequestandRecallOptionsinDocs.pyandSessions.py..github/workflows/generate-changelog.yml.This description was created by
for d43cc8b. It will automatically update as commits are pushed.