fix: fix overriding prefetch limit in local mode#1081
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughqdrant_client/local/local_collection.py: query_groups now deep-copies the provided prefetch structure and applies limits via a new iterative routine. The recursive helper was removed and replaced by set_prefetch_limit_iteratively(prefetch, limit), which walks nested Prefetch objects (and lists) using an explicit stack and mutates .limit in-place, returning None. tests/congruence_tests/test_query.py: adds dense_queries_rescore_group_single_prefetch to exercise a nested single-outer-prefetch scenario and updates test_query_group to run and compare results against the existing rescoring flow. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)qdrant_client/local/local_collection.py (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (1)
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 |
joein
force-pushed
the
fix-local-query-points-groups
branch
from
d5c929d to
ca239dd
Compare
| def set_prefetch_limit_recursively(prefetch: types.Prefetch, limit: int) -> types.Prefetch: | ||
| if prefetch is not None: | ||
| def set_prefetch_limit_recursively( | ||
| prefetch: Union[types.Prefetch, list[types.Prefetch]], limit: int | ||
| ) -> None: | ||
| if isinstance(prefetch, list): | ||
| for p in prefetch: | ||
| set_prefetch_limit_recursively(p, limit) | ||
| elif isinstance(prefetch, types.Prefetch): | ||
| prefetch.limit = limit | ||
|
|
||
| if isinstance(prefetch.prefetch, list): | ||
| return types.Prefetch( | ||
| limit=limit, | ||
| prefetch=[set_prefetch_limit_recursively(p, limit) for p in prefetch.prefetch], | ||
| ) | ||
| else: | ||
| return types.Prefetch(limit=limit, prefetch=list()) | ||
| for p in prefetch.prefetch: | ||
| set_prefetch_limit_recursively(p, limit) | ||
| elif isinstance(prefetch.prefetch, types.Prefetch): | ||
| set_prefetch_limit_recursively(prefetch.prefetch, limit) |
There was a problem hiding this comment.
I'm not sure if we really care, but there should probably be a cycle check/max depth or something here, or at least a try/except for RecursionError with a instructive message. Because not only could prefetch be arbitrarily deeply nested, you can also technically put a prefetch object into the list inside itself.
There was a problem hiding this comment.
I think since this is user-controlled there definitely should be something here.
| """Set .limit on all nested Prefetch objects without recursion.""" | ||
| stack: list[Union[types.Prefetch, list[types.Prefetch]]] = [prefetch] | ||
|
|
||
| while stack: |
There was a problem hiding this comment.
It could still get stuck in an infinite loop I guess, but I guess putting a prefetch object in its own list won't happen on accident, so it's fine.
* fix: fix overriding prefetch limit in local mode * fix: replace recursion with a while loop in prefetch
2 participants
#1078