Skip to content

fix: preserve explicit cuda selection in parallel workers - #764

Closed
RAMZI0TO99 wants to merge 1 commit into
qdrant:mainfrom
RAMZI0TO99:fix/parallel-cuda-forwarding
Closed

RAMZI0TO99 wants to merge 1 commit into
qdrant:mainfrom
RAMZI0TO99:fix/parallel-cuda-forwarding

Conversation

@RAMZI0TO99

Copy link
Copy Markdown
Contributor

ParallelWorkerPool.start only forwards the pool's cuda option when
device_ids is nonempty. For example, a pool constructed with cuda=False
and no device_ids starts workers without a cuda argument. ONNX model
constructors then use their Device.AUTO default, losing the caller's CPU
selection. With an available CUDA provider, that can select GPU execution;
the reproduction below verifies the missing option, without GPU inference.

Always copy self.cuda into each worker's startup options, and keep the
existing round-robin device_id assignment. The pool constructor's cuda
setting now consistently takes precedence over a conflicting cuda in
start/ordered_map kwargs, including when device_ids is absent or empty.
Explicit providers retain their existing precedence in model selection.

Add regression coverage using the actual pool and reporting workers:
False, True, Device.CPU, Device.CUDA and Device.AUTO; absent, empty and
explicit device IDs; preservation of other startup options; constructor
precedence; ordered results; and round-robin assignment. The tests use real
child processes for forwarding and result checks, and inspect Process
creation arguments for deterministic round-robin coverage. They select
spawn and forkserver when supported by the platform.

Reproduction: save this as a Python file and run it from a development
environment. The main guard is required for spawn, including on Windows.
No GPU or model download is needed.

from fastembed.parallel_processor import ParallelWorkerPool, Worker


class ReportingWorker(Worker):
    def __init__(self, options):
        self.options = options

    @classmethod
    def start(cls, **kwargs):
        return cls(kwargs)

    def process(self, items):
        for index, _ in items:
            yield index, self.options


if __name__ == "__main__":
    pool = ParallelWorkerPool(
        1, ReportingWorker, start_method="spawn", cuda=False
    )
    received = list(pool.ordered_map(["report"]))[0]
    print("Worker received:", received)
    assert received.get("cuda") is False, "CPU selection was not forwarded"

Before:

Worker received: {}
AssertionError: CPU selection was not forwarded

After, matching the expected result:

Worker received: {'cuda': False}

Environment: Windows 11 build 10.0.26200, Python 3.13.5, NumPy 2.3.5,
pytest 9.1.1, ONNX Runtime 1.30.0 (CPU distribution), FastEmbed 0.8.1 at
upstream main commit 7d36728. The
reproduction and tests import the real FastEmbed implementation.

Validation:

  • Before the fix: 12 new cases failed and eight passed.
  • After: 28 passed cleanly on Python 3.13.5, including all 20 new cases
    and the existing parallel-processor and common tests. Command:
    python -m pytest tests/test_parallel_cuda_forwarding.py tests/test_parallel_processor.py tests/test_common.py -q
  • Ruff 0.3.4 lint/format and whitespace checks pass.
  • Python 3.10.11 also reports all 20 new cases passing with exit code zero,
    but emits Windows access-violation diagnostics during process creation.
    The same diagnostics reproduce in a standalone standard-library-only
    multiprocessing program without FastEmbed or pytest. All 12 child
    processes across four diagnostic probes exit zero; the native cause is
    unidentified, so this is not claimed as clean Python 3.10 validation.
  • Local process coverage is spawn on Windows. Forkserver, GPU inference,
    and the full model-download suite were not run locally.

All Submissions

  • Followed the guidelines in the Contributing document.
  • Checked for other open PRs for the same change: no equivalent fix
    found in eight all-state GitHub searches or the complete file diffs of
    all 33 open PRs reviewed. Related historical PRs Multi gpu support #358 and new: use cuda if available #537 introduce
    device assignment and Device.AUTO defaults; neither fixes this omission.

@RAMZI0TO99
RAMZI0TO99 requested a review from joein as a code owner October 3, 2026 14:51
@coderabbitai

coderabbitai Bot commented Oct 3, 2026

Copy link
Copy Markdown

Review in Change Stack →

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 configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a4de548f-9403-49ae-b520-db1e29d67cbd
📥 Commits

Reviewing files that changed from the base of the PR and between 7d36728 and 95139df.

📒 Files selected for processing (2)
  • fastembed/parallel_processor.py
  • tests/test_parallel_cuda_forwarding.py

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


📝 Walkthrough

Walkthrough

ParallelWorkerPool.start now passes the configured cuda value to each worker, including when no device IDs are configured. When device IDs are configured, the existing round-robin assignment remains in place. New tests cover CUDA forwarding, device ID assignment, pool CUDA precedence over per-call options, multiprocessing start methods, and ordered results.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 95139

This change makes parallel workers respect an explicit CUDA setting even when no device IDs are configured. No merge-blocking risk was found, and new tests cover the behavior.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 95139

The change preserves the caller’s CPU or GPU selection without adding privileges or bypassing explicit execution-provider settings. No material security risk was identified in the reviewed paths.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated exposure is execution-provider selection for workers created by the configured pool. The change uses the existing process target and device-ID source rather than adding a new privilege-granting mechanism. Deployment-specific tenant and device isolation are not established by this source evidence.

Trust Boundaries and Controls

  • observed — Pool-owned CUDA configuration crosses the existing parent-to-worker option boundary. Explicit provider configuration is checked before CUDA inference, so the forwarded CUDA option does not override that selection control.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ 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. Analyzed 8 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preserving explicit CUDA selection in parallel workers.
Description check ✅ Passed The description explains the CUDA forwarding issue, the fix, and the regression tests. It is directly related to the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · 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.

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.

1 participant