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

No multiprocessing #1256

Merged
merged 30 commits into from Feb 8, 2022
Merged

No multiprocessing #1256

merged 30 commits into from Feb 8, 2022

Conversation

GaelVaroquaux
Copy link
Member

A continuation of #1246 to make joblib robust to non working multiprocessing.

@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #1256 (b0b3663) into master (978bd90) will increase coverage by 0.10%.
The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1256      +/-   ##
==========================================
+ Coverage   88.38%   88.49%   +0.10%     
==========================================
  Files          47       50       +3     
  Lines        7052     7118      +66     
==========================================
+ Hits         6233     6299      +66     
  Misses        819      819              
Impacted Files Coverage Δ
joblib/parallel.py 96.55% <89.47%> (+0.13%) ⬆️
joblib/test/test_parallel.py 95.74% <96.87%> (-0.03%) ⬇️
joblib/__init__.py 100.00% <100.00%> (ø)
joblib/_cloudpickle_wrapper.py 100.00% <100.00%> (ø)
joblib/_multiprocessing_helpers.py 88.46% <100.00%> (+8.46%) ⬆️
joblib/test/test_cloudpickle_wrapper.py 100.00% <100.00%> (ø)
joblib/test/test_memmapping.py 99.25% <100.00%> (+<0.01%) ⬆️
joblib/test/test_missing_multiprocessing.py 100.00% <100.00%> (ø)
joblib/test/test_module.py 100.00% <100.00%> (ø)
joblib/_parallel_backends.py 91.54% <0.00%> (-0.37%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 978bd90...b0b3663. Read the comment docs.

Copy link
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

Looks nice with the test! thx @GaelVaroquaux

joblib/test/test_memmapping.py Show resolved Hide resolved
@rth
Copy link
Contributor

rth commented Feb 4, 2022

Thanks! It's indeed better to address this on the joblib side rather than in loky.

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM as well. Thanks for the fix.

I pushed a merge of master to run the tests with the updated CI config. There are still some failures on master but that should run the tests on more plaforms.

@ogrisel
Copy link
Contributor

ogrisel commented Feb 4, 2022

Please document the change in the changelog before merging.

@GaelVaroquaux
Copy link
Member Author

I rebased on master. Will document the changes

@GaelVaroquaux
Copy link
Member Author

OK, I have added the functionality to fall back to threading when multiprocessing is not available.

There are still some failing tests, but I suspect that they are failing on master, so I will rebase on master when they are fixed.

@ogrisel
Copy link
Contributor

ogrisel commented Feb 7, 2022

I sync'ed master to check the CI.

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

There is an actual problem in the warning: see the failed CI.

Also, the coverage report indicates that test_backend_no_multiprocessing is never executed on our CI. Indeed I am not sure when this test is supposed to be executed. Have you run it locally with pyiodine?

If so do you think we should have a new CI config for this? Maybe it's not worth the extra maintenance effort and test_missing_multiprocessing is enough?

doc/parallel.rst Outdated Show resolved Hide resolved
joblib/parallel.py Outdated Show resolved Hide resolved
GaelVaroquaux and others added 2 commits February 7, 2022 21:10
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@GaelVaroquaux
Copy link
Member Author

The test_backend_no_multiprocessing should have been run on the linux_py37_no_multiprocessing_no_lzma) CI configuration. Let me look into why that didn't happen.

@GaelVaroquaux
Copy link
Member Author

The test_backend_no_multiprocessing should have been run on the linux_py37_no_multiprocessing_no_lzma) CI configuration. Let me look into why that didn't happen.

It works as it should, and actually captured legit errors which I fixed. Let's see what this gives.

@GaelVaroquaux
Copy link
Member Author

All green! (took me a while). This should be ready to merge.

@ogrisel ogrisel merged commit a64b808 into joblib:master Feb 8, 2022
@ogrisel
Copy link
Contributor

ogrisel commented Feb 8, 2022

Merged. Thanks very much @hoodmane and @GaelVaroquaux!

@hoodmane
Copy link
Contributor

hoodmane commented Feb 10, 2022

Thanks @GaelVaroquaux for completing this! Much appreciated.

@rth rth mentioned this pull request May 12, 2022
2 tasks
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 22, 2022
Release 1.2.0
Fix a security issue where eval(pre_dispatch) could potentially run arbitrary code. Now only basic numerics are supported. joblib/joblib#1327
Make sure that joblib works even when multiprocessing is not available, for instance with Pyodide joblib/joblib#1256
Avoid unnecessary warnings when workers and main process delete the temporary memmap folder contents concurrently. joblib/joblib#1263
Fix memory alignment bug for pickles containing numpy arrays. This is especially important when loading the pickle with mmap_mode != None as the resulting numpy.memmap object would not be able to correct the misalignment without performing a memory copy. This bug would cause invalid computation and segmentation faults with native code that would directly access the underlying data buffer of a numpy array, for instance C/C++/Cython code compiled with older GCC versions or some old OpenBLAS written in platform specific assembly. joblib/joblib#1254
Vendor cloudpickle 2.2.0 which adds support for PyPy 3.8+.
Vendor loky 3.3.0 which fixes several bugs including:
robustly forcibly terminating worker processes in case of a crash (joblib/joblib#1269);
avoiding leaking worker processes in case of nested loky parallel calls;
reliability spawn the correct number of reusable workers.
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.

None yet

5 participants