perf(retrieval): skim every head alongside the section ranking; read 30 pages, not 40 (HAL-1566) - #71
Open
Conversation
Navigation ran four sequential levels: rank sections, skim the heads of the chosen sections' pages, read the best in full, follow a cross-reference. The skim waited on the ranking only because of which pages it read. A page's head score does not depend on the ranking, so under SkimAll every page's head is scored in the same round as the ranking and one level goes away. What is read in full is unchanged: the ranked sections' pages up to CoarsePages, ordered by the same scores, which a test holds equal to the sequential skim. It costs the heads of pages outside the chosen sections, so it is on only where every page is already in memory (the persisted-pages path), and only for documents up to SkimAllMaxPages (400). A longer document falls back to the sequential skim. RankLeaves sends its batches together when a tree has more leaves than one request holds, as rankPages already did. navbench takes -skim-all.
Full pages are about 60% of a query's tokens. Measured on 40 FinanceBench questions with the head skim running alongside the section ranking, repeated: 40 pages hit 35/36/35 $0.00431/q 30 pages hit 36/36/36 $0.00388/q median 1.8-2.0 s 20 pages hit 35/36 $0.00345/q All three sit inside Jev's run-to-run noise. 30 keeps headroom for answers spread over several pages, which FinanceBench barely tests and a smaller read would hurt first. With it, the faster navigation also costs less than the sequential one did ($0.00402). navbench's -pages now defaults to the engine's own budget instead of restating 40.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideThis PR removes a sequential retrieval stage by concurrently ranking sections and skimming page heads for persisted-page documents up to 400 pages, reuses the same scores to choose full reads, batches leaf-ranking requests concurrently, lowers the default full-read budget to 30 pages, and adds navbench controls plus regression tests. Sequence diagram for concurrent persisted-page navigationsequenceDiagram
participant Strategy as JudgeWalkStrategy
participant Navigator as JudgeNavigator
participant LeafRanker as RankLeaves
participant HeadRanker as rankPages
participant Reader as FullPageReader
Strategy->>Navigator: Navigate(query, leaves)
Navigator->>Navigator: allLeafPages(loadPages)
par Section ranking
Navigator->>LeafRanker: RankLeaves(query, leaves)
LeafRanker->>LeafRanker: Judge.Judge(batch requests)
and Page-head skim
Navigator->>HeadRanker: rankPages(query, all pages, headChars)
end
Navigator->>Navigator: Select ranked-section candidates by head scores
Navigator->>Reader: Read up to maxPages (default 30)
Reader-->>Strategy: Navigation result
Flow diagram for SkimAll fallback and full-read selectionflowchart TD
A["Persisted pages navigation"] --> B{"SkimAll enabled?"}
B -- No --> C["Rank sections"]
B -- Yes --> D{"Page count <= 400 and > full-read budget?"}
D -- No --> C
D -- Yes --> E["Load all leaf pages"]
E --> F["Concurrently rank sections and skim every page head"]
F --> G["Keep ranked-section candidates"]
G --> H["Order candidates by head score"]
H --> I["Read top 30 pages in full"]
C --> J["Rank heads for selected pages"]
J --> I
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pkg/retrieval/judgewalk.go" line_range="495-503" />
<code_context>
+ rankErr, headErr error
+ )
+ wg.Add(2)
+ go func() {
+ defer wg.Done()
+ ranked, lu, lr, rankErr = n.RankLeaves(ctx, query, leaves)
+ }()
+ go func() {
+ defer wg.Done()
+ heads, hu, hr, headErr = n.rankPages(ctx, query, all, n.headChars(), true)
+ }()
+ wg.Wait()
+ if rankErr != nil {
+ return nil, fmt.Errorf("judgewalk: rank leaves: %w", rankErr)
</code_context>
<issue_to_address>
**issue (bug_risk):** When either the concurrent section-ranking or head-skimming operation fails, the other operation keeps running because each ranking method creates and owns a separate child context; `Navigate` only waits for both and never cancels the sibling. The remaining requests therefore continue consuming provider capacity and tokens after the navigation has already become unrecoverable.
**Triggers:** When one of the two concurrent ranking operations returns an error while the other still has in-flight requests.
**Suggested fix:** Create one cancellable child context in `Navigate` and pass it to both operations, or use an `errgroup.Group` so either error cancels the sibling immediately.
</issue_to_address>
Comment on lines
+495
to
+503
| go func() { | ||
| defer wg.Done() | ||
| ranked, lu, lr, rankErr = n.RankLeaves(ctx, query, leaves) | ||
| }() | ||
| go func() { | ||
| defer wg.Done() | ||
| heads, hu, hr, headErr = n.rankPages(ctx, query, all, n.headChars(), true) | ||
| }() | ||
| wg.Wait() |
There was a problem hiding this comment.
issue (bug_risk): When either the concurrent section-ranking or head-skimming operation fails, the other operation keeps running because each ranking method creates and owns a separate child context; Navigate only waits for both and never cancels the sibling. The remaining requests therefore continue consuming provider capacity and tokens after the navigation has already become unrecoverable.
Triggers: When one of the two concurrent ranking operations returns an error while the other still has in-flight requests.
Suggested fix: Create one cancellable child context in Navigate and pass it to both operations, or use an errgroup.Group so either error cancels the sibling immediately.
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.
Why
With the guard fixed, a query is four sequential levels at ~0.6 s each. The head skim waited on the section ranking only to learn which pages to skim.
What
JudgeNavigator.SkimAll: every page's head is scored in the same round as the section ranking. The full read is chosen from the same candidates by the same scores, and a test holds it equal to the sequential path. It's on only in the persisted-pages path (every page is already in memory) and for documents up to 400 pages.RankLeavessends multiple batches together, asrankPagesalready did.-skim-all;-pagesdefaults to the engine budget.Evidence
40 FinanceBench questions, repeated runs (evaluation §3):
The same two borderline questions flip in every mode, so the accuracy differences are noise. The shipped default is faster and cheaper than the sequential original.
Not yet measured:
/v1/queryend to end over HTTP and the database.Tests: ranking and skim are in flight together; same pages read as the sequential path at 5/10/40 budgets; fallback over the page cap; RankLeaves batches sent together.
go test -racegreen locally. CI blocked (HAL-1712).Stacked on #69.
Closes HAL-1566
Summary by Sourcery
Accelerate document navigation by overlapping ranking work and page-head skimming while lowering the default full-read budget.
New Features:
Enhancements:
Tests: