-
Notifications
You must be signed in to change notification settings - Fork 100
feat: paralell tool execution #166
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
Open
jirastorza
wants to merge
4
commits into
main
Choose a base branch
from
feat/paralell_tool_execution
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+46
−23
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
4e1f8d4
feat: paralellize tool execution.
jirastorza af7ac38
Merge branch 'main' into feat/paralell_tool_execution
jirastorza 2c0b0f0
fix: error message and tool messages ordering.
jirastorza edf54b4
fix: try-except modified on _run_tools
jirastorza File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If chunk_spans is empty, the content field will be {"documents": []}, which might be fine, but might not be.
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.
We should confirm if retrieve_context can return 0 chunks.
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.
I did not change this behavior with respect to the main branch.
Without metadata_filter, I think retrieve_context always returns a list of ChunkSpans, even if they are not that relevant (low similarity). With a metadata_filter applied (f.e. using self-query), the list could be empty.
I don't know if an empty list should be an issue, I find it more correct than retrieving non-relevant chunks for a query that does not have related documents on the database.
Open to discuss this :)
@Robbe-Superlinear
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.
I agree that retrieve_context should return an empty list when no relevant chunks are found after applying metadata filtering.
However, how should we handle this case in the response? Currently, we return a message like this:
We could consider a few alternatives for better clarity:
Option 1:
Option 2:
Option 3: We could return None and let the _run_tools function handle empty tool_call responses.