Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delay initialization of multiprocessing.Lock until it's needed #615

Merged

Conversation

neutrinoceros
Copy link
Contributor

[Description of PR]

Fixes #522
I'm not sure how to properly test this though, as the test would need to run before numcodecs is even imported the first time, which seems hard to control with how pytest collects all tests before running any particular one.

TODO:

  • Unit tests and/or doctests in docstrings
  • Tests pass locally
  • [N/A] Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • Docs build locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Codecov passes)

@neutrinoceros neutrinoceros force-pushed the avoid_mp_locking_at_importtime branch from 9b5de74 to d7a4363 Compare October 23, 2024 14:19
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.91%. Comparing base (e3299b8) to head (45cbf87).
Report is 56 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #615   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files          59       59           
  Lines        2405     2405           
=======================================
  Hits         2403     2403           
  Misses          2        2           

Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@dstansby dstansby enabled auto-merge (squash) October 28, 2024 22:36
@dstansby dstansby merged commit 230bc01 into zarr-developers:main Oct 28, 2024
29 checks passed
@jakirkham
Copy link
Member

Thank you for fixing this long standing bug! 🙏

Would be curious to hear how you arrived at this solution?

@neutrinoceros neutrinoceros deleted the avoid_mp_locking_at_importtime branch October 29, 2024 06:13
@neutrinoceros
Copy link
Contributor Author

I picked up a suggestion from astropy/astropy#16351 (comment), so credit goes to @saimn for figuring it out. I'm just the muscle here 😅

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.

Using multiprocessing.set_start_method() after importing numcodecs results in an exception
3 participants