Skip to content

fix(Multiprocessing): Fully disable multiprocessing when not used#281

Merged
nicola-bastianello merged 8 commits intoteam-decent:mainfrom
Simpag:test-checkpoints-fix
Mar 31, 2026
Merged

fix(Multiprocessing): Fully disable multiprocessing when not used#281
nicola-bastianello merged 8 commits intoteam-decent:mainfrom
Simpag:test-checkpoints-fix

Conversation

@Simpag
Copy link
Copy Markdown
Contributor

@Simpag Simpag commented Mar 30, 2026

This PR fully disables multiprocessing when max_processes = 1. This speeds up execution on windows based systems as the overhead of spawning processing using spawn context is large. For this reason, this PR also disabled the multiprocessing checkpointing tests for all systems not running Linux since Linux by default uses fork which comes with less overhead. Finally, a better error message is given when users runs unguarded benchmarks (main script is not wrapped in if __name__ == '__main__') while using multiprocessing (max_processes > 1).

closes #279

Copilot AI review requested due to automatic review settings March 30, 2026 18:05
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reduces multiprocessing overhead by avoiding multiprocessing.Manager()/spawn setup when max_processes == 1, adjusts progress-bar coordination to work without a manager, and updates checkpointing tests to skip multi-process cases on non-Linux platforms.

Changes:

  • Disable multiprocessing initialization (manager/log listener/spawn context) when max_processes == 1.
  • Allow ProgressBarController to run with manager=None by using a local in-process queue.
  • Skip max_processes > 1 checkpointing test variants on non-Linux, and refactor CPU-core skip logic.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
decent_bench/benchmark/_benchmark.py Avoid multiprocessing init for single-process runs; improve spawn main-guard error message; make log listener optional.
decent_bench/utils/progress_bar.py Support manager=None by falling back to a local queue to avoid manager overhead.
test/utils/test_checkpoints.py Skip multi-process checkpoint tests on non-Linux; centralize CPU-count-based skipping.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@adrianardv
Copy link
Copy Markdown
Contributor

Maybe it would also be worth adding a short note in the user guide about the Windows spawn behavior and the if name == "main": guard?

@Simpag
Copy link
Copy Markdown
Contributor Author

Simpag commented Mar 30, 2026

Maybe it would also be worth adding a short note in the user guide about the Windows spawn behavior and the if name == "main": guard?

Good idea, the user docs are very outdated but its still good so we remember to include it

@Simpag
Copy link
Copy Markdown
Contributor Author

Simpag commented Mar 30, 2026

I have not touched anything with the documentation and RTD fails... Why is complaining now about the class tagger?

@nicola-bastianello
Copy link
Copy Markdown
Member

thank you for fixing this and the RTD issue!

I confirm the tests now take very little time on my machine too (<10s). merging now

@nicola-bastianello nicola-bastianello merged commit 55d26b1 into team-decent:main Mar 31, 2026
7 checks passed
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.

Windows-specific pytest slowdown in test/utils/test_checkpoints.py

4 participants