fix: set NUM_WORKERS=0 for non-fork multiprocessing start methods #4784
Conversation
📝 WalkthroughWalkthroughThe change introduces a runtime check in Changes
Sequence Diagram(s)sequenceDiagram
participant EnvModule as env.py
participant OS as Operating System
participant Logger as logging
EnvModule->>OS: Get multiprocessing start method
OS-->>EnvModule: Return start method ("fork", "spawn", etc.)
EnvModule->>EnvModule: If start method != "fork"
EnvModule->>Logger: Log warning about NUM_WORKERS and start method
EnvModule->>EnvModule: Set NUM_WORKERS = 0
Assessment against linked issues
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)deepmd/pt/utils/env.pyNo files to lint: exiting. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #4686 by ensuring that NUM_WORKERS is set to 0 when the multiprocessing start method is not set to "fork."
- Added logging and multiprocessing imports.
- Introduced a condition to override NUM_WORKERS based on the start method.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/pt/utils/env.py (1)
27-34: LGTM! Solid fix for DataLoader compatibility on non-fork platforms.The logic correctly identifies non-fork multiprocessing start methods and sets NUM_WORKERS to 0 for PyTorch DataLoader compatibility. This addresses the core issue on platforms like macOS where "fork" is unavailable.
Consider logging the warning only when NUM_WORKERS was originally > 0 to avoid unnecessary warnings:
if multiprocessing.get_start_method() != "fork": - # spawn or forkserver does not support NUM_WORKERS > 0 for DataLoader - log = logging.getLogger(__name__) - log.warning( - "NUM_WORKERS > 0 is not supported with spawn or forkserver start method. " - "Setting NUM_WORKERS to 0." - ) - NUM_WORKERS = 0 + # spawn or forkserver does not support NUM_WORKERS > 0 for DataLoader + if NUM_WORKERS > 0: + log = logging.getLogger(__name__) + log.warning( + "NUM_WORKERS > 0 is not supported with spawn or forkserver start method. " + "Setting NUM_WORKERS to 0." + ) + NUM_WORKERS = 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/pt/utils/env.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Analyze (python)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test C++ (true)
- GitHub Check: Test C++ (false)
🔇 Additional comments (1)
deepmd/pt/utils/env.py (1)
2-3: LGTM! Necessary imports for multiprocessing compatibility.The logging and multiprocessing imports are correctly placed and necessary for the new functionality to check start methods and log warnings.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4784 +/- ##
=======================================
Coverage 84.79% 84.80%
=======================================
Files 698 698
Lines 67775 67793 +18
Branches 3544 3542 -2
=======================================
+ Hits 57472 57489 +17
- Misses 9169 9171 +2
+ Partials 1134 1133 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fix #4686
Summary by CodeRabbit