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

tests: Use fork instead of forkserver #174

Merged

Conversation

bugant
Copy link
Contributor

@bugant bugant commented Feb 11, 2025

Make sure test can be successfully run on pyton 3.14.

According to
https://docs.python.org/dev/whatsnew/3.14.html#multiprocessing

The default start method (see Contexts and start methods) changed from fork to forkserver on platforms other than macOS & Windows where it was already spawn. If you require the threading incompatible fork start method you must explicitly request it using a context from multiprocessing.get_context() (preferred) or change the default via multiprocessing.set_start_method().

See also https://bugzilla.redhat.com/show_bug.cgi?id=2326934 for reference on the reported bug for the Fedora package.

Pull Request check-list

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?

Description of change

@bugant bugant force-pushed the use-multiprocessing-fork-instead-of-forkserver branch from 8514c61 to 5fe7e54 Compare February 11, 2025 16:21
@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.26%. Comparing base (95c4b68) to head (76de3d1).

Files with missing lines Patch % Lines
tests/test_multiprocessing.py 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #174      +/-   ##
==========================================
- Coverage   76.26%   76.26%   -0.01%     
==========================================
  Files         130      130              
  Lines       33964    33970       +6     
==========================================
+ Hits        25903    25906       +3     
- Misses       8061     8064       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bugant bugant force-pushed the use-multiprocessing-fork-instead-of-forkserver branch from 5fe7e54 to ab1c205 Compare February 13, 2025 08:09
Make sure test can be successfully run on pyton 3.14.

According to
https://docs.python.org/dev/whatsnew/3.14.html#multiprocessing

The default start method (see Contexts and start methods) changed from
fork to forkserver on platforms other than macOS & Windows where it was
already spawn. If you require the threading incompatible fork start
method you must explicitly request it using a context from
multiprocessing.get_context() (preferred) or change the default via
multiprocessing.set_start_method().

See also https://bugzilla.redhat.com/show_bug.cgi?id=2326934 for
reference on the reported bug for the Fedora package.

Signed-off-by: Matteo Centenaro <[email protected]>
@bugant bugant force-pushed the use-multiprocessing-fork-instead-of-forkserver branch from ab1c205 to 76de3d1 Compare February 13, 2025 13:58
@mkmkme mkmkme self-assigned this Feb 18, 2025
Copy link
Collaborator

@mkmkme mkmkme left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the swift fix. It will be part of 6.1.1

@mkmkme mkmkme merged commit fd0e9b7 into valkey-io:main Feb 20, 2025
93 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants