Skip to content

Conversation

@jirastorza
Copy link

@jirastorza jirastorza commented Oct 22, 2025

Pull Request Overview

This pull request introduces parallel execution for tool calls in the RAG pipeline to improve performance when multiple tools are invoked simultaneously. The refactoring splits the tool execution logic into separate functions and leverages Python's ThreadPoolExecutor for concurrent processing.

Key changes:

  • Refactored monolithic _run_tools function into _run_tool (single execution) and _run_tools (parallel orchestration)
  • Implemented parallel tool execution using ThreadPoolExecutor with configurable worker count

@emilradix emilradix requested a review from Copilot October 22, 2025 08:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces parallel execution for tool calls in the RAG pipeline to improve performance when multiple tools are invoked simultaneously. The refactoring splits the tool execution logic into separate functions and leverages Python's ThreadPoolExecutor for concurrent processing.

Key changes:

  • Refactored monolithic _run_tools function into _run_tool (single execution) and _run_tools (parallel orchestration)
  • Implemented parallel tool execution using ThreadPoolExecutor with configurable worker count
  • Added error handling with future cancellation to prevent resource leaks on failures

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +155 to +164
chunk_spans = retrieve_context(**kwargs)
message = {
"role": "tool",
"content": '{{"documents": [{elements}]}}'.format(
elements=", ".join(
chunk_span.to_json(index=i + 1) for i, chunk_span in enumerate(chunk_spans)
)
),
"tool_call_id": tool_call.id,
}

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.

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.

Copy link
Author

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

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:

{
    "role": "tool",
    "content": "{documents: []}", 
    "tool_call_id": tool_call.id
}

We could consider a few alternatives for better clarity:
Option 1:

{
    "role": "tool",
    "content": "{documents: [], message: No results found}", 
    "tool_call_id": tool_call.id
}

Option 2:

{
    "role": "tool",
    "content": "{message: No results found}", 
    "tool_call_id": tool_call.id
}

Option 3: We could return None and let the _run_tools function handle empty tool_call responses.

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.

3 participants