Skip to content

Reject nonpositive parallel worker counts - #818

Merged
joein merged 3 commits into
qdrant:mainfrom
IlyaKhalafi:fix/positive-worker-count
Oct 28, 2026
Merged

joein merged 3 commits into
qdrant:mainfrom
IlyaKhalafi:fix/positive-worker-count

Conversation

@IlyaKhalafi

Copy link
Copy Markdown
Contributor

This PR fixes nonpositive worker counts waiting for a worker that was never started. With this, ParallelWorkerPool rejects them immediately with ValueError. This also applies to empty input. The public parallel=0 option still selects the CPU count.

Fixes #817.

12 focused tests, pre-commit and the CI mypy command passed. The existing early-close worker test was excluded because it also fails on upstream in this environment.

@IlyaKhalafi
IlyaKhalafi requested a review from joein as a code owner October 14, 2026 10:35
@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9c8c423c-438d-46d5-b279-a56aab6ec250
Commits

Reviewing files that changed from the base of the PR and between 0220a20 and 5543247.

Files selected for processing: 2
  • fastembed/parallel_processor.py
  • tests/test_parallel_processor.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.

Walkthrough

ParallelWorkerPool.__init__ now raises ValueError when num_workers is zero or negative. A parameterized test checks those values and the error message.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related PRs

  • qdrant/fastembed#815: Initializes processes before construction can fail, which relates to cleanup when the pool constructor raises.
Merge Risk: Minimal · up to 55432

The worker-count validation matches the requested behavior and is ready to merge.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: rejecting zero and negative parallel worker counts.
Description check Passed The description directly explains the worker-count validation, the parallel=0 behavior, tests, and linked issue. It is related to the changeset.
Linked Issues check Passed [#817] Requirement 1: reject zero and negative worker counts with ValueError before queue allocation. At head, ParallelWorkerPool.init raises ValueError("num_workers must be greater than zero, got…
Out of Scope Changes check Passed The PR changes two files, according to the supplied raw summary: fastembed/parallel_processor.py (+2/-0, the nonpositive num_workers check) and tests/test_parallel_processor.py (+7/-0, the parametrize…
Full details: Linked Issues check

Explanation

[#817] Requirement 1: reject zero and negative worker counts with ValueError before queue allocation. At head, ParallelWorkerPool.init raises ValueError("num_workers must be greater than zero, got ...") when num_workers <= 0. Queues are created only in start(), so the check runs before any queue allocation. Empty input is also rejected, because the check runs in the constructor before input is consumed. Requirement 2: public parallel=0 selects automatic CPU selection. fastembed/text/onnx_text_model.py _embed_documents sets parallel = os.cpu_count() when parallel == 0, and passes num_workers=parallel or 1 to the pool, so the pool size is positive. Negative public parallel values now reach the new ValueError. Test coverage: tests/test_parallel_processor.py adds a parametrized test for worker counts 0, -1, and -2 that asserts ValueError with the required message. The author reports that 12 focused tests passed. This review did not execute tests. The existing early-close worker test is not part of this issue and was not assessed.

Full details: Out of Scope Changes check

Explanation

The PR changes two files, according to the supplied raw summary: fastembed/parallel_processor.py (+2/-0, the nonpositive num_workers check) and tests/test_parallel_processor.py (+7/-0, the parametrized rejection test). Both changes implement issue #817 directly. No unrelated change appears in the supplied evidence. Scope uncertainty: the whole-PR text diff and the merge-base blob for fastembed/parallel_processor.py could not be read, so the changed-file list and line counts rely on the supplied AI-generated summary. The head file shows the added check, and the summary reports no other changes to that file.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@joein
joein force-pushed the fix/positive-worker-count branch from 0220a20 to 5543247 Compare October 28, 2026 18:37
Removed test for non-positive worker counts in ParallelWorkerPool.
@joein
joein merged commit 60fe942 into qdrant:main Oct 28, 2026
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.

Non-positive worker counts wait for results without starting workers

2 participants