Repository navigation
fix: preserve explicit cuda selection in parallel workers - #764
RAMZI0TO99 wants to merge 1 commit into
Conversation
|
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
📒 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: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to 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 ReviewSecurity architecture risk: ⚪ Minimal · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 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 |
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.
Before:
After, matching the expected result:
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:
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 -qbut 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.
and the full model-download suite were not run locally.
All Submissions
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.