Skip to content

bpo-40692: Run more test_concurrent_futures tests #20239

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

Merged
merged 3 commits into from
Feb 8, 2021

Conversation

paulproteus
Copy link
Contributor

@paulproteus paulproteus commented May 19, 2020

In the case of the multiprocessing.synchronize module being missing, the
test_concurrent_futures test suite now skips only the tests that
require multiprocessing.synchronize.

Validate that multiprocessing.synchronize exists as part of
_check_system_limits(), allowing ProcessPoolExecutor to raise
NotImplementedError during init, rather than crashing with
ImportError during init when creating a lock imported from
multiprocessing.synchronize.

Use _check_system_limits() to disable tests of
ProcessPoolExecutor on systems without multiprocessing.synchronize.

Running the test suite without multiprocessing.synchronize reveals
that Lib/compileall.py crashes when it uses a ProcessPoolExecutor.
Therefore, change Lib/compileall.py to call _check_system_limits()
before creating the ProcessPoolExecutor.

Note that both Lib/compileall.py and Lib/test/test_compileall.py
were attempting to sanity-check ProcessPoolExecutor by expecting
ImportError. I cannot see any situation where importing
ProcessPoolExecutor would result in ImportError, so I replaced
both checks with a check for NotImplementedError from
_check_system_limits().

In multiprocessing.resource_tracker, sem_unlink() is also absent
on platforms where POSIX semaphores aren't available. Avoid using
sem_unlink() if it, too, does not exist.

https://bugs.python.org/issue40692

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@paulproteus

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@1st1
Copy link
Member

1st1 commented May 19, 2020

@pablogsal Pablo, not sure but I think this is in your wheelhouse.

@pablogsal pablogsal self-assigned this May 19, 2020
@pablogsal
Copy link
Member

@pablogsal Pablo, not sure but I think this is in your wheelhouse.

Oh boy, oh boy, multiprocessing: here we go again :)

Thanks for the ping @1st1! I will take a look soon at the PR once I finish some pending stuff

@csabella
Copy link
Contributor

@paulproteus, please sign the CLA as requested by the @the-knights-who-say-ni bot. Thanks!

@paulproteus
Copy link
Contributor Author

Hi @csabella ! I believe I followed the process on the day I submitted the pull request, and I can email you the PDF I got from the Adobe signature system if that helps. I think there may be a delay in processing it somewhere. Happy to sign it again if that may help. Let me know the best way to proceed. Cheers!

@csabella
Copy link
Contributor

@paulproteus, looking at the checker and at the bug tracker, it doesn't show that the contributor form was received. The names need to match exactly, so maybe there was a discrepancy the first time you did it. Would you be able to try again? Thanks!

@paulproteus
Copy link
Contributor Author

Hey all - I faced some problems with the contributor agreement due to having multiple Roundup accounts. I'm going to get that sorted out and come back to this. Thanks.

In the case of multiprocessing.synchronize() being missing, the
test_concurrent_futures test suite now skips only the tests that
require multiprocessing.synchronize().

Validate that multiprocessing.synchronize exists as part of
_check_system_limits(), allowing ProcessPoolExecutor to raise
NotImplementedError during __init__, rather than crashing with
ImportError during __init__ when creating a lock imported from
multiprocessing.synchronize.

Use _check_system_limits() to disable tests of
ProcessPoolExecutor on systems without multiprocessing.synchronize.

Running the test suite without multiprocessing.synchronize reveals
that Lib/compileall.py crashes when it uses a ProcessPoolExecutor.
Therefore, change Lib/compileall.py to call _check_system_limits()
before creating the ProcessPoolExecutor.

Note that both Lib/compileall.py and Lib/test/test_compileall.py
were attempting to sanity-check ProcessPoolExecutor by expecting
ImportError. I cannot see any situation where importing
ProcessPoolExecutor would result in ImportError, so I replaced
both checks with a check for NotImplementedError from
_check_system_limits().

In multiprocessing.resource_tracker, sem_unlink() is also absent
on platforms where POSIX semaphores aren't available. Avoid using
sem_unlink() if it, too, does not exist.
@paulproteus
Copy link
Contributor Author

On my macOS dev system, I ran:

./configure ac_cv_posix_semaphores_enabled=no && make clean && make -j && make test

And I got this output in the end:

== Tests result: SUCCESS ==

405 tests OK.

21 tests skipped:
    test_devpoll test_epoll test_gdb test_ioctl test_msilib
    test_multiprocessing_fork test_multiprocessing_forkserver
    test_multiprocessing_main_handling test_multiprocessing_spawn
    test_ossaudiodev test_smtpnet test_spwd test_ssl test_startfile
    test_tix test_tk test_ttk_guionly test_winconsoleio test_winreg
    test_winsound test_zipfile64

Whereas before this patch, if I run the same commands, I get:

3 tests failed again:
    test_compileall test_fcntl test_logging

== Tests result: FAILURE then FAILURE ==

401 tests OK.

3 tests failed:
    test_compileall test_fcntl test_logging

22 tests skipped:
    test_concurrent_futures test_devpoll test_epoll test_gdb
    test_ioctl test_msilib test_multiprocessing_fork
    test_multiprocessing_forkserver test_multiprocessing_main_handling
    test_multiprocessing_spawn test_ossaudiodev test_smtpnet test_spwd
    test_ssl test_startfile test_tix test_tk test_ttk_guionly
    test_winconsoleio test_winreg test_winsound test_zipfile64

3 re-run tests:
    test_compileall test_fcntl test_logging

Total duration: 4 min 3 sec
Tests result: FAILURE then FAILURE
make: *** [test] Error 2

This means that with this patch, with ./configure ac_cv_posix_semaphores_enabled=no, we get the following improvements:

  • 0 failing tests
  • test_concurrent_futures is no longer being skipped

For me, I'm interested in the ac_cv_posix_semaphores_enabled=no case because Android is an example platform with ac_cv_posix_semaphores_enabled=no.

Happy to discuss more! I could probably have been more concise. Apologies for that.

@paulproteus
Copy link
Contributor Author

@csabella @pablogsal I've fixed my CLA issues and the merge conflict issues and would love to get review on this. Happy to discuss. Thanks!

@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 7, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 6b5a545 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 7, 2021
@@ -16,10 +16,14 @@
import unittest

from unittest import mock, skipUnless
from concurrent.futures import ProcessPoolExecutor
Copy link
Member

@pablogsal pablogsal Feb 7, 2021

Choose a reason for hiding this comment

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

Why is this import outside the try-except?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ProcessPoolExecutor can always be imported successfully, see e.g.

class ProcessPoolExecutor(_base.Executor):

If the import were within the try-except it would still happen successfully, so it doesn't seem to improve anything to do it within the try-except. From what I can tell, the try-except exists to catch an exception from _check_system_limits().

Since it's peaceful to always import it, and I think it's simpler that way, I moved it out of the try-except.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks for clarifiying

@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 7, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 2cea2b1 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 7, 2021
@pablogsal pablogsal merged commit bf2e7e5 into python:master Feb 8, 2021
@pablogsal
Copy link
Member

pablogsal commented Feb 8, 2021

Thanks for the PR, @paulproteus 👍

@paulproteus paulproteus deleted the issue40692 branch February 8, 2021 03:18
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
In the case of multiprocessing.synchronize() being missing, the
test_concurrent_futures test suite now skips only the tests that
require multiprocessing.synchronize().

Validate that multiprocessing.synchronize exists as part of
_check_system_limits(), allowing ProcessPoolExecutor to raise
NotImplementedError during __init__, rather than crashing with
ImportError during __init__ when creating a lock imported from
multiprocessing.synchronize.

Use _check_system_limits() to disable tests of
ProcessPoolExecutor on systems without multiprocessing.synchronize.

Running the test suite without multiprocessing.synchronize reveals
that Lib/compileall.py crashes when it uses a ProcessPoolExecutor.
Therefore, change Lib/compileall.py to call _check_system_limits()
before creating the ProcessPoolExecutor.

Note that both Lib/compileall.py and Lib/test/test_compileall.py
were attempting to sanity-check ProcessPoolExecutor by expecting
ImportError. In multiprocessing.resource_tracker, sem_unlink() is also absent
on platforms where POSIX semaphores aren't available. Avoid using
sem_unlink() if it, too, does not exist.

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
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.

6 participants