⚡ Bolt: [performance improvement] Replace O(N²) nested loop with O(N) hash map lookup in reranker#360
Open
bashandbone wants to merge 1 commit into
Open
Conversation
…p in reranker output processing Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces an O(N²) batch rank lookup in the reranker output transformer with an O(N) dictionary-based lookup, and documents this performance pattern in the Bolt guidelines. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
🤖 Hi @bashandbone, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
🤖 I'm sorry @bashandbone, but I was unable to process your request. Please see the logs for more details. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the new Bolt guideline, consider softening the wording from “Always precompute a hash map…” to something scoped to large N or hot paths, so it doesn’t encourage premature optimization in cases where a simpler linear scan is clearer and N is small.
- Since
mapped_scoresis now only used to buildrank_lookup, you could build the lookup directly fromresults(e.g., sorting indices byresults[idx]) to avoid the intermediate(idx, score)tuples and keep the transformation a bit more direct.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new Bolt guideline, consider softening the wording from “Always precompute a hash map…” to something scoped to large N or hot paths, so it doesn’t encourage premature optimization in cases where a simpler linear scan is clearer and N is small.
- Since `mapped_scores` is now only used to build `rank_lookup`, you could build the lookup directly from `results` (e.g., sorting indices by `results[idx]`) to avoid the intermediate `(idx, score)` tuples and keep the transformation a bit more direct.
## Individual Comments
### Comment 1
<location path=".jules/bolt.md" line_range="29-30" />
<code_context>
+## 2024-05-18 - Avoid O(N²) nested loops with generator comprehensions in list building
</code_context>
<issue_to_address>
**suggestion (typo):** Consider changing “generator comprehensions” to the more precise Python term “generator expressions.”
This aligns with the standard pairing of “list comprehension” and “generator expression” in Python terminology, which will be clearer to readers.
Suggested implementation:
```
## 2024-05-18 - Avoid O(N²) nested loops with generator expressions in list building
```
```
**Learning:** In list comprehensions or extending lists (e.g. `extend(...)`), using a generator expression like `next((... for x in y if ...))` inside the loop over `N` elements creates an $O(N^2)$ algorithmic complexity if `y` also has `N` elements. This happened in `default_reranking_output_transformer` where `batch_rank` was calculated.
```
</issue_to_address>
### Comment 2
<location path=".jules/bolt.md" line_range="30" />
<code_context>
**Action:** Always favor using the walrus operator `:=` in list comprehensions or conditionals when identical string manipulations (e.g., `.strip()`) or expensive evaluation calls appear repeatedly within the identical expression branch.
+
+## 2024-05-18 - Avoid O(N²) nested loops with generator comprehensions in list building
+**Learning:** In list comprehensions or extending lists (e.g. `extend(...)`), using a generator comprehension like `next((... for x in y if ...))` inside the loop over `N` elements creates an $O(N^2)$ algorithmic complexity if `y` also has `N` elements. This happened in `default_reranking_output_transformer` where `batch_rank` was calculated.
+**Action:** Always precompute a hash map (dictionary lookup) for mapping values outside the loop to change the complexity from $O(N^2)$ to $O(N)$, especially in hot paths like reranking output processing.
</code_context>
<issue_to_address>
**nitpick (typo):** Minor grammar tweak: “In list comprehensions or extending lists” could read more smoothly.
Consider rephrasing to: “In list comprehensions or when extending lists (e.g., `extend(...)`), …” for smoother grammar and clarity.
```suggestion
**Learning:** In list comprehensions or when extending lists (e.g. `extend(...)`), using a generator comprehension like `next((... for x in y if ...))` inside the loop over `N` elements creates an $O(N^2)$ algorithmic complexity if `y` also has `N` elements. This happened in `default_reranking_output_transformer` where `batch_rank` was calculated.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+29
to
+30
| ## 2024-05-18 - Avoid O(N²) nested loops with generator comprehensions in list building | ||
| **Learning:** In list comprehensions or extending lists (e.g. `extend(...)`), using a generator comprehension like `next((... for x in y if ...))` inside the loop over `N` elements creates an $O(N^2)$ algorithmic complexity if `y` also has `N` elements. This happened in `default_reranking_output_transformer` where `batch_rank` was calculated. |
Contributor
There was a problem hiding this comment.
suggestion (typo): Consider changing “generator comprehensions” to the more precise Python term “generator expressions.”
This aligns with the standard pairing of “list comprehension” and “generator expression” in Python terminology, which will be clearer to readers.
Suggested implementation:
## 2024-05-18 - Avoid O(N²) nested loops with generator expressions in list building
**Learning:** In list comprehensions or extending lists (e.g. `extend(...)`), using a generator expression like `next((... for x in y if ...))` inside the loop over `N` elements creates an $O(N^2)$ algorithmic complexity if `y` also has `N` elements. This happened in `default_reranking_output_transformer` where `batch_rank` was calculated.
| **Action:** Always favor using the walrus operator `:=` in list comprehensions or conditionals when identical string manipulations (e.g., `.strip()`) or expensive evaluation calls appear repeatedly within the identical expression branch. | ||
|
|
||
| ## 2024-05-18 - Avoid O(N²) nested loops with generator comprehensions in list building | ||
| **Learning:** In list comprehensions or extending lists (e.g. `extend(...)`), using a generator comprehension like `next((... for x in y if ...))` inside the loop over `N` elements creates an $O(N^2)$ algorithmic complexity if `y` also has `N` elements. This happened in `default_reranking_output_transformer` where `batch_rank` was calculated. |
Contributor
There was a problem hiding this comment.
nitpick (typo): Minor grammar tweak: “In list comprehensions or extending lists” could read more smoothly.
Consider rephrasing to: “In list comprehensions or when extending lists (e.g., extend(...)), …” for smoother grammar and clarity.
Suggested change
| **Learning:** In list comprehensions or extending lists (e.g. `extend(...)`), using a generator comprehension like `next((... for x in y if ...))` inside the loop over `N` elements creates an $O(N^2)$ algorithmic complexity if `y` also has `N` elements. This happened in `default_reranking_output_transformer` where `batch_rank` was calculated. | |
| **Learning:** In list comprehensions or when extending lists (e.g. `extend(...)`), using a generator comprehension like `next((... for x in y if ...))` inside the loop over `N` elements creates an $O(N^2)$ algorithmic complexity if `y` also has `N` elements. This happened in `default_reranking_output_transformer` where `batch_rank` was calculated. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes reranking output post-processing by eliminating an O(N²) rank lookup inside default_reranking_output_transformer, improving performance for large rerank batches. It also records the performance lesson in the Bolt playbook.
Changes:
- Precompute a
rank_lookupdictionary from sorted scores to assignbatch_rankin O(1) per result (overall O(N) after sorting). - Replace the per-item
next(...enumerate(mapped_scores)...)search with a dictionary.get()lookup. - Add a Bolt guideline documenting the “avoid nested generator lookups causing O(N²)” pattern and recommended fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/codeweaver/providers/reranking/providers/base.py |
Replaces per-item linear scan for rank with a precomputed dictionary lookup. |
.jules/bolt.md |
Documents the performance guideline motivating the change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
💡 What: In$N$ items to find the rank of each item from a list of $N$ items. This was replaced with a precomputed hash map ($O(N^2)$ algorithmic complexity because it performed a linear scan for each element in the worst case. The new implementation precomputes a lookup table, bringing the total complexity down to $O(N)$ .
default_reranking_output_transformer, a generator comprehension withnext()was used inside a loop overrank_lookup).🎯 Why: The previous implementation had an
📊 Impact: Expected performance improvement in processing outputs for large numbers of reranked chunks. Testing locally showed significant speedups for
N=500and higher.🔬 Measurement: Verify the logic by looking at tests for
RerankingProviderimplementations or runningmise //:test tests/unit/providers.PR created automatically by Jules for task 1239632412642915857 started by @bashandbone
Summary by Sourcery
Optimize reranking output transformation by replacing quadratic-time rank computation with a linear-time dictionary-based lookup and document the performance guideline in the Bolt playbook.
Enhancements:
default_reranking_output_transformerto avoid O(N²) generator-based searches when assigningbatch_rank.