Repository navigation
Raise ValueError for non-positive batch_size in iter_batch - #721
Conversation
iter_batch passed batch_size straight into islice with no lower-bound check. A batch_size of 0 made every islice call return an empty list immediately, so embed and rerank returned an empty result with no error across every modality that shares this helper. A negative batch_size hit islice's own argument validation and raised a cryptic ValueError instead of a clear one. Validate batch_size >= 1 once in iter_batch itself, since every embed and rerank entrypoint across dense text, sparse text, image, late-interaction and cross-encoder rerank funnels through it. Added test_iter_batch_rejects_non_positive_size and test_iter_batch_accepts_positive_size to tests/test_common.py. The new rejection test fails on unmodified code (batch_size=0 returns silently instead of raising) and passes with the fix. Fixes qdrant#719
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add assertions for the promised batch-size-specific error message.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Adds centralized validation to reject non-positive batch_size values in iter_batch.
Changes:
- Raises a clear
ValueErrorfor invalid batch sizes. - Adds tests for positive and non-positive sizes.
- Tests should also assert the expected error message for both invalid values.
| File | Summary |
|---|---|
tests/test_common.py |
Adds batch-size validation tests; message assertions are still needed. |
fastembed/common/utils.py |
Validates batch size before batching. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| with pytest.raises(ValueError): | ||
| list(iter_batch([1, 2, 3], 0)) | ||
|
|
||
| with pytest.raises(ValueError): | ||
| list(iter_batch([1, 2, 3], -1)) |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthrough
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Batch-size errors are clearer, and valid batching remains lazy. No merge-blocking risk is identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Non-positive batch sizes now raise an error instead of producing an empty result or a less clear error. The change is shared across model APIs, but the reviewed paths show no new access, privilege, or deployment behavior. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
hey @adityaanikam thanks for taking care of it! I've made some fixes so the check would work correctly in the parallel setting |
Fixes #719
iter_batch(fastembed/common/utils.py) passedbatch_sizestraight intoislicewith no lower bound check.islice(iterable, 0)returns nothing on every call, sobatch_size=0madeembedandreranksilently return an empty result, with no error, across every modality that shares this helper (dense text, sparse text, image, late-interaction, and cross-encoder rerank). A negativebatch_sizehitislice's own argument validation and raised a crypticValueErrorinstead of a clear one.This adds a single validation check at the top of
iter_batch, so it applies once for every caller instead of needing to be duplicated at each public entrypoint.Testing: added
test_iter_batch_rejects_non_positive_sizeandtest_iter_batch_accepts_positive_sizetotests/test_common.py. The rejection test fails on unmodifiedmain(batch_size=0returns without raising) and passes with the fix.