gh-108388: Convert test_concurrent_futures to package by vstinner · Pull Request #108401 · python/cpython · GitHub

Convert test_concurrent_futures to a package of 7 sub-tests.

Add remote_globals to create_executor_tests()

Issue: Split long running test suites up into smaller sub suites to reduce our regrtest long tail on multi-core systems. #108388

Convert test_concurrent_futures to a package of 7 sub-tests. Add remote_globals to create_executor_tests()

Timing of running tests: ./python -m test test_concurrent_futures -j0 --slowest on a machine with 12 threads / 6 CPU cores.

reference: 2 min 43 sec
PR: 1 min 13 sec

With the PR:

10 slowest tests:
- test_concurrent_futures.test_wait: 1 min 13 sec
- test_concurrent_futures.test_process_pool: 42.6 sec
- test_concurrent_futures.test_shutdown: 20.1 sec
- test_concurrent_futures.test_as_completed: 12.0 sec
- test_concurrent_futures.test_deadlock: 11.1 sec
- test_concurrent_futures.test_future: 3.2 sec
- test_concurrent_futures.test_init: 2.3 sec

To fix the CI

GHA job timings:

Windows x86: 24 min 37 sec
Windows x64: 19 min 27 sec
macOS: 18 min 22 sec
Ubuntu: 10 min
Address Sanitizer: 14 min 49 sec

Details.

Windows x86:

10 slowest tests:
- test_importlib: 3 min 39 sec
- test.test_multiprocessing_spawn.test_manager: 2 min 49 sec
- test_compileall: 2 min 47 sec
- test_venv: 2 min 33 sec
- test_tarfile: 2 min 28 sec
- test.test_multiprocessing_spawn.test_processes: 2 min 10 sec
- test_threading: 2 min 2 sec
- test_fstring: 1 min 56 sec
- test_queue: 1 min 52 sec
- test_socket: 1 min 43 sec

Windows x64:

10 slowest tests:
- test.test_multiprocessing_spawn.test_processes: 3 min 18 sec
- test.test_multiprocessing_spawn.test_manager: 2 min 36 sec
- test_regrtest: 2 min 27 sec
- test_importlib: 2 min 6 sec
- test_compileall: 2 min 5 sec
- test_venv: 2 min 2 sec
- test_zipfile: 1 min 57 sec
- test_io: 1 min 47 sec
- test_tarfile: 1 min 45 sec
- test_largefile: 1 min 38 sec

macOS:

10 slowest tests:
- test.test_multiprocessing_spawn.test_processes: 3 min 20 sec
- test_tarfile: 2 min 48 sec
- test.test_multiprocessing_forkserver.test_processes: 2 min 19 sec
- test_pickle: 1 min 58 sec
- test_cppext: 1 min 40 sec
- test_statistics: 1 min 39 sec
- test_compileall: 1 min 25 sec
- test.test_concurrent_futures.test_wait: 1 min 24 sec
- test_subprocess: 1 min 22 sec
- test.test_multiprocessing_spawn.test_misc: 1 min 21 sec

Ubuntu:

10 slowest tests:
- test_gdb: 2 min 23 sec
- test.test_multiprocessing_spawn.test_processes: 1 min 31 sec
- test.test_concurrent_futures.test_wait: 1 min 22 sec
- test_signal: 1 min 2 sec
- test.test_multiprocessing_forkserver.test_processes: 1 min 1 sec
- test_subprocess: 1 min
- test_weakref: 44.6 sec
- test_venv: 44.2 sec
- test_socket: 42.1 sec
- test.test_concurrent_futures.test_process_pool: 41.4 sec

Address Sanitizer:

10 slowest tests:
- test_subprocess: 2 min 11 sec
- test_compileall: 2 min 9 sec
- test_venv: 2 min
- test_gdb: 1 min 39 sec
- test_weakref: 1 min 32 sec
- test_tarfile: 1 min 26 sec
- test_regrtest: 1 min 12 sec
- test_source_encoding: 1 min 11 sec
- test_multiprocessing_main_handling: 1 min 1 sec
- test_decimal: 1 min

I believe you unintentionally got rid of most of the Threading tests.

(do not merge this even after fixing up this review round's comments, this requires further very close side by side review that Github's UI is incapable of providing as it lacks the ability to track file splits and moves as a diff of what's changed -- I'll need to spend time on the command line in a client looking things over)

Otherwise: Thank you for taking this on! This test suite has needed refactoring into something manageable forever.

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

I moved this PR to Draft state to signal that I don't want anyone to think it is ready to merge even if someone approves it until after we've taken a close look to verify that we haven't lost anything in the refactoring.

I believe you unintentionally got rid of most of the Threading tests.

I compared ./python -m test test_concurrent_futures --list-cases|wc -l with and without my change: I get 248 test cases in both cases.

For your comments like "ThreadPoolExecutorTest is defined above but missing here" on test_process_pool.py, I suggest you looking at which tests are executed rather than only looking at the code. For example:

$ ./python -m test test_concurrent_futures --list-cases|grep ThreadPoolExecutorTest
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_default_workers
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_executor_map_current_future_cancel
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_free_reference
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_hang_global_shutdown_lock
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_idle_thread_reuse
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_map
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_map_exception
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_map_submits_without_iteration
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_map_timeout
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_max_workers_negative
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_no_stale_references
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_saturation
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_shutdown_race_issue12456
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_submit
test.test_concurrent_futures.test_process_pool.ThreadPoolExecutorTest.test_submit_keyword

You can compare between the main branch and my PR: I get the same 15 tests, but with my PR, test names also get an additional test_process_pool. in their name.

I splitted test_process_pool in two testrs:

test_thread_pool: 15 tests, takes 10.6 sec
test_process_pool: 60 tests, takes 32.0 sec

Before, test_process_poll was the sum (75 tests, ~42 sec) 🙂.

Thanks. confirmed locally as well, all tests are present and accounted for!

Thanks for the review @gpshead! For sure, there is room for improvement for these tests. I hope that this split will ease future maintenance.

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

Sorry, @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker aa6f787faa4bc45006da4dc2f942fb9b82c98836 3.12

GH-108443 is a backport of this pull request to the 3.12 branch.

the merge conflict on this is probably just the list in regrtest runtests.py. retry that automation after the test_multiprocessing one is in 3.12.

…08401) Convert test_concurrent_futures to a package of sub-tests. (cherry picked from commit aa6f787)

the merge conflict on this is probably just the list in regrtest runtests.py. retry that automation after the test_multiprocessing one is in 3.12.

Right. I backported the change to Python 3.12. There is a minor conflict in libregrtest on:

 SPLITTESTDIRS = {
     "test_asyncio",
+    "test_concurrent_futures",
 }

The backport is in conflict with PR #108401 (split multiprocessing tests) which may be merged first.

#108443) gh-108388: Convert test_concurrent_futures to package (#108401) Convert test_concurrent_futures to a package of sub-tests. (cherry picked from commit aa6f787)

GH-109704 is a backport of this pull request to the 3.11 branch.

Since I was asked to backport all test changes (including refactoring) to stable branches, I backport this change to Python 3.11: GH-109704.

#109704) * gh-108388: Convert test_concurrent_futures to package (#108401) Convert test_concurrent_futures to a package of sub-tests. (cherry picked from commit aa6f787) Notes on backport to 3.11: * AsCompletedTests: Revert test_future_times_out() => test_zero_timeout() * Restore TODO comment * ThreadPoolExecutorTest.test_hang_global_shutdown_lock(): add @support.requires_resource('cpu').

pythongh-108388 : Convert test_concurrent_futures to package

56d01d0

Convert test_concurrent_futures to a package of 7 sub-tests. Add remote_globals to create_executor_tests()

vstinner added the skip news label Aug 24, 2023

bedevere-bot mentioned this pull request Aug 24, 2023

Split long running test suites up into smaller sub suites to reduce our regrtest long tail on multi-core systems. #108388

Closed

bedevere-bot added the awaiting core review label Aug 24, 2023

Fix Lint

69759e0

vstinner mentioned this pull request Aug 24, 2023

gh-108388: test_concurrent_futures requires cpu #108389

Closed

sobolevn reviewed Aug 24, 2023

View reviewed changes

Comment thread Lib/test/test_concurrent_futures/test_shutdown.py Outdated

Fix Lint job

868c6d3

corona10 self-requested a review August 24, 2023 14:05

gpshead requested changes Aug 24, 2023

View reviewed changes

Comment thread Lib/test/test_concurrent_futures/test_process_pool.py

Comment thread Lib/test/test_concurrent_futures/test_shutdown.py

Comment thread Lib/test/test_concurrent_futures/test_wait.py

Comment thread Lib/test/test_concurrent_futures/test_deadlock.py

bedevere-bot added awaiting changes and removed awaiting core review labels Aug 24, 2023

gpshead self-assigned this Aug 24, 2023

gpshead marked this pull request as draft August 24, 2023 15:34

bedevere-bot removed the awaiting changes label Aug 24, 2023

Split test_process_pool: add test_thread_pool

4e9ceb5

gpshead approved these changes Aug 24, 2023

View reviewed changes

bedevere-bot added the awaiting merge label Aug 24, 2023

gpshead marked this pull request as ready for review August 24, 2023 17:08

bedevere-bot added awaiting core review and removed awaiting merge labels Aug 24, 2023

vstinner merged commit aa6f787 into python:main Aug 24, 2023

bedevere-bot removed the awaiting core review label Aug 24, 2023

vstinner deleted the test_concurrent_package branch August 24, 2023 17:21

gpshead added the needs backport to 3.12 only security fixes label Aug 24, 2023

miss-islington assigned vstinner and unassigned gpshead Aug 24, 2023

bedevere-bot removed the needs backport to 3.12 only security fixes label Aug 24, 2023

vstinner added a commit to vstinner/cpython that referenced this pull request Aug 24, 2023

pythongh-108388 : Convert test_concurrent_futures to package (python#1…

d377273

…08401) Convert test_concurrent_futures to a package of sub-tests. (cherry picked from commit aa6f787)

vstinner mentioned this pull request Aug 24, 2023

[3.12] gh-108388: Convert test_concurrent_futures to package (#108401) #108443

Merged

vstinner mentioned this pull request Sep 22, 2023

[3.11] gh-108388: Convert test_concurrent_futures to package (#108401) #109704

Merged

gh-108388: Convert test_concurrent_futures to package by vstinner · Pull Request #108401 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-108388: Convert test_concurrent_futures to package - #108401

gh-108388: Convert test_concurrent_futures to package#108401vstinner merged 4 commits intopython:mainpython/cpython:mainfrom vstinner:test_concurrent_packagevstinner/cpython:test_concurrent_packageCopy head branch name to clipboard

Conversation

vstinner commented Aug 24, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Aug 24, 2023

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Aug 24, 2023

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Aug 24, 2023

Uh oh!

gpshead commented Aug 24, 2023

Uh oh!

vstinner commented Aug 24, 2023

Uh oh!

vstinner commented Aug 24, 2023

Uh oh!

vstinner commented Aug 24, 2023

Uh oh!

gpshead commented Aug 24, 2023

Uh oh!

vstinner commented Aug 24, 2023

Uh oh!

miss-islington commented Aug 24, 2023

Uh oh!

miss-islington commented Aug 24, 2023

Uh oh!

bedevere-bot commented Aug 24, 2023

Uh oh!

gpshead commented Aug 24, 2023

Uh oh!

vstinner commented Aug 24, 2023

Uh oh!

bedevere-app Bot commented Sep 22, 2023

Uh oh!

vstinner commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gh-108388: Convert test_concurrent_futures to package#108401
vstinner merged 4 commits into
python:mainfrom
vstinner:test_concurrent_package

vstinner commented Aug 24, 2023 •
edited by bedevere-bot
Loading

vstinner commented Sep 22, 2023 •
edited
Loading