| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…ai#7918) import deepspeed eagerly calls is_compatible() for all ops; eight builders probed get_device_properties(0), which lazy-inits CUDA and breaks fork()-based multiprocessing. Gate the probe on is_initialized() via a shared CUDAOpBuilder.cuda_capability_major() helper, and clarify that pytest --forked is safe now that import no longer initializes a CUDA context. Fixes deepspeedai#7918 Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
This PR addresses a fork-safety issue where import deepspeed could initialize a CUDA context (via import-time op compatibility checks), breaking fork()-based multiprocessing. It introduces a fork-safe CUDA capability probe and updates CUDA op builders to avoid context creation during import.
Changes:
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file| File | Description |
|---|---|
| tests/unit/ops/test_op_builder.py | Adds unit tests for the new helper and a subprocess regression test to ensure import deepspeed doesn’t initialize CUDA. |
| op_builder/builder.py | Introduces CUDAOpBuilder.cuda_capability_major() with guards to avoid CUDA context initialization. |
| op_builder/transformer_inference.py | Switches capability checks to the fork-safe helper and gates comparisons on None. |
| op_builder/spatial_inference.py | Switches Ampere gating to the fork-safe helper and guards on None. |
| op_builder/ragged_utils.py | Switches capability checks to the fork-safe helper and guards on None. |
| op_builder/ragged_ops.py | Switches capability checks to the fork-safe helper and guards on None. |
| op_builder/inference_cutlass_builder.py | Switches capability checks to the fork-safe helper and guards on None. |
| op_builder/inference_core_ops.py | Switches capability checks to the fork-safe helper and guards on None. |
| op_builder/fp_quantizer.py | Switches capability checks to the fork-safe helper and guards on None. |
| op_builder/evoformer_attn.py | Switches capability checks to the fork-safe helper and guards on None. |
| docs/contributing.md | Updates contributing guidance to clarify that --forked is safe now that imports don’t initialize CUDA. |
| CONTRIBUTING.md | Mirrors the contributing guidance update from docs/contributing.md. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
| check = ( | ||
| "import torch, deepspeed; " | ||
| "assert not torch.cuda.is_initialized(), " #ignore-cuda | ||
| "'import deepspeed initialized a CUDA context (issue #7918)'") | ||
| result = subprocess.run([sys.executable, "-c", check], capture_output=True, text=True) | ||
| if "ModuleNotFoundError" in result.stderr: | ||
| pytest.skip("deepspeed/torch not importable in a subprocess in this environment") | ||
| assert result.returncode == 0, result.stderr |
There was a problem hiding this comment.
Replaced this test — the new one sets explicit PYTHONPATH, a timeout, and only skips on No module named 'deepspeed'/'torch' or a genuinely absent GPU.
Sorry, something went wrong.
There was a problem hiding this comment.
Here are some automated review suggestions for this pull request.
Reviewed commit: 02b1c335cd
ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Sorry, something went wrong.
| "import torch, deepspeed; " | ||
| "assert not torch.cuda.is_initialized(), " #ignore-cuda |
There was a problem hiding this comment.
Verify fork safety, not just CUDA context state
This regression check can pass while the fork failure still exists: import deepspeed still runs op compatibility checks that call torch.cuda.is_available(), and PyTorch only documents that call as non-poisoning when PYTORCH_NVML_BASED_CUDA_CHECK=1 is set (https://docs.pytorch.org/docs/stable/generated/torch.cuda.is_available.html). Since is_available() can poison fork without making torch.cuda.is_initialized() true, CUDA-enabled environments can still fail in a forked child even though this assertion succeeds; the test should actually fork after import and touch CUDA, or the import path must avoid/use the NVML-safe availability check.
Useful? React with 👍 / 👎.
Sorry, something went wrong.
There was a problem hiding this comment.
Addressed — is_available() poisons via cuInit by default without setting is_initialized(), so the old assertion was a false green. deepspeed/init.py now sets PYTORCH_NVML_BASED_CUDA_CHECK=1, and the test forks after import and has the child use CUDA.
Sorry, something went wrong.
|
Hi @Achyuthan-S, thank you for opening this PR! Could you add a regression test for that fork-after-import behavior and update the fix until that test passes? |
Sorry, something went wrong.
torch.cuda.is_available() runs cudaGetDeviceCount/cuInit by default, creating a CUDA context at 'import deepspeed' (accelerator auto-detect and every op builder's is_compatible() call it). That poisons fork()-based multiprocessing without ever setting torch.cuda.is_initialized(), so the previous import-time assertion passed while the fork still failed. Opt into PyTorch's NVML-based availability check (PYTORCH_NVML_BASED_CUDA_CHECK=1) so is_available() no longer initializes a CUDA context; combined with the existing get_device_properties guard, importing DeepSpeed leaves CUDA uninitialized. Replace the weak import-time check with a fork-after-import regression test that a forked child can use CUDA. Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
|
HI @tohtana , thank you for the review . You're correct — the earlier change only guarded get_device_properties, while torch.cuda.is_available() (called during accelerator detection and in every builder's is_compatible()) was the real poison, since it runs cuInit by default without flipping is_initialized(). Pushed a fix that opts into PYTORCH_NVML_BASED_CUDA_CHECK=1 at import and adds a fork-after-import regression test (child uses CUDA in a forked subprocess). |
Sorry, something went wrong.
|
Thanks for the update. I tested the current head in a CUDA environment, and I think the original issue is still not fixed: after import deepspeed in the parent process, CUDA is already initialized, so a forked child still fails when it first touches CUDA with Cannot re-initialize CUDA in forked subprocess. |
Sorry, something went wrong.
ds_transformer gated its triton-kernel import on is_triton_supported(), which reads the GPU compute capability and thereby creates a CUDA context at 'import deepspeed' (issue deepspeedai#7918) — the real initializer on a GPU node with triton installed. Import the triton kernels whenever triton is available; actual triton use remains gated at runtime by config.use_triton, where CUDA is already initialized. With this, importing DeepSpeed no longer initializes CUDA and a forked child can use CUDA. Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
|
Good catch @tohtana — the real initializer was is_triton_supported() called at import from ds_transformer.py (reads compute capability → creates a CUDA context). I've dropped the import-time capability probe (triton kernels are imported on HAS_TRITON; their use stays gated by config.use_triton at runtime). Verified on a GPU node: after import deepspeed, is_initialized() is False and a forked child uses CUDA successfully. |
Sorry, something went wrong.
|
Hello @tohtana, could you please verify and let me know , I think it should work now |
Sorry, something went wrong.
Signed-off-by: Masahiro Tanaka <mtanaka@anyscale.com>
There was a problem hiding this comment.
Hi @Achyuthan-S,
Sorry for the delay. I also confirmed it worked on my environment. As I see a conflict, let me quickly fix it and merge this PR.
This was a high-impact fix. Thank you for your contribution!
Sorry, something went wrong.
Hi @tohtana , Thank you very much for your support. Please feel free to assign me to interesting issues/features. would love to contribute more! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
import deepspeed initialized a CUDA context in the parent process, which permanently breaks fork()-based multiprocessing (Cannot re-initialize CUDA in forked subprocess). This makes importing DeepSpeed fork-safe.
Fixes #7918.
Root cause
On a GPU box, import deepspeed reached three distinct calls that create a CUDA context, each gated differently (which is why a single patch kept missing one):
Fix
Behavior / tradeoff
Tests
Validation
Verified on a CUDA GPU node (NVIDIA, torch 2.4.1+cu121). After import deepspeed:
Docs
Updated CONTRIBUTING.md and docs/contributing.md: --forked is safe now that import deepspeed no longer initializes CUDA.
cc @tjruwase @loadams @tohtana