GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-108388: test_concurrent_futures requires cpu by vstinner · Pull Request #108389 · python/cpython

Conversation

@vstinner

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The test_concurrent_futures and test_multiprocessing_spawn tests now require the 'cpu' resource. Skip these tests unless the 'cpu' resource is enabled (it is disabled by default).

test_concurrent_futures is no longer skipped if Python is built with ASAN or MSAN sanitizer.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
pythongh-108388: test_concurrent_futures requires cpu
83becc5
The test_concurrent_futures and test_multiprocessing_spawn tests now
require the 'cpu' resource. Skip these tests unless the 'cpu'
resource is enabled (it is disabled by default).

test_concurrent_futures is no longer skipped if Python is built with
ASAN or MSAN sanitizer.
@bedevere-bot bedevere-bot mentioned this pull request Aug 23, 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 bedevere-bot added tests Tests in the Lib/test dir awaiting core review labels Aug 23, 2023
@vstinner

vstinner commented Aug 23, 2023

Copy link
Copy Markdown
Member Author

I already made the change for test_peg_generator and test_freeze in PR #108386, since it's less important to test the PEG generator and the freeze tool at each commit. Buildbots run these tests as post-commit (with the cpu resource enabled).

make buildbottest, make testall, make testuniversal, and make hostrunnertest run the Python test suite with -u all.

On the other hand, make test and ./python -m test does not pass -u all option and so the cpu resource is disabled.

Buildbots run test Python suite with make buildbottest which use the -u all option.

See also the buildbot configuration: https://github.com/python/buildmaster-config/blob/main/master/custom/factories.py

The following buildbot factories disable the cpu resource:

  • UnixRefleakBuild: -R 3:3 -u-cpu
  • WindowsRefleakBuild: -u-cpu
  • SlowWindowsBuild: -u-cpu -u-largefile

These factories enable it explicitly, even if it's already enabled by make buildbottest:

  • UnixInstalledBuild: -uall
  • Windows64BigmemBuild: -uall

There are other ways to run the Python test suite:

  • (Windows) PCbuild/rt.bat doesn't use -uall
  • (Windows) Tools/buidbot/test.bat uses -uall

The Fedora package to build Python uses the python -m test command without passing -uall: https://src.fedoraproject.org/rpms/python3.12/blob/rawhide/f/python3.12.spec#_1138

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Aug 23, 2023

Copy link
Copy Markdown
Member Author

Last year, issue #90682 discussed solutions to make the CI faster:

  • Shard tests i.e. run tests on two different jobs concurrently
  • Start the slower tests first

The issue title was "test_peg_generator takes 8 minutes on Windows" and it was closed when Gregory disabled compiler optimizations on Windows in test_peg_generator: the test takes 3-5 min instead of 5-16 min.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Aug 23, 2023

Copy link
Copy Markdown
Member Author

Total test duration, before (PR #108386) => after (this PR):

  • Windows x86: 19 min 26 sec => 20 min 12 sec (+40 sec)
  • Windows x64: 30 min 34 sec (ignore this one, test_concurrent_futures was re-run!) => 17 min 40 sec (ignored)
  • macOS: 17 min 34 sec => 12 min 26 sec (-5 min)
  • Ubuntu: 12 min 18 sec => 10 min 24 sec (-2 min)
  • Address Sanitizer: 15 min 24 sec => 14 min 6 sec (-1 min)

The exact time depends if slowest tests are run first, since tests are randomized.

Details of timing on this PR.

Windows x86:

10 slowest tests:
- test_compileall: 2 min 54 sec
- test_importlib: 2 min 38 sec
- test_tarfile: 2 min 14 sec
- test_regrtest: 1 min 49 sec
- test_venv: 1 min 46 sec
- test_threading_local: 1 min 44 sec
- test_fstring: 1 min 43 sec
- test_zipfile: 1 min 40 sec
- test_socket: 1 min 40 sec
- test_threading: 1 min 35 sec

Windows x64:

10 slowest tests:
- test_importlib: 3 min 4 sec
- test_venv: 2 min 2 sec
- test_regrtest: 1 min 56 sec
- test_compileall: 1 min 49 sec
- test_fstring: 1 min 46 sec
- test_tarfile: 1 min 45 sec
- test_socket: 1 min 33 sec
- test_queue: 1 min 31 sec
- test_mmap: 1 min 28 sec
- test_io: 1 min 22 sec

macOS:

10 slowest tests:
- test_signal: 2 min 18 sec
- test_multiprocessing_forkserver: 2 min 11 sec
- test_tarfile: 2 min 1 sec
- test_largefile: 1 min 41 sec
- test_ssl: 1 min 37 sec
- test_pickle: 1 min 35 sec
- test_cppext: 1 min 22 sec
- test_compileall: 1 min 19 sec
- test_venv: 1 min 14 sec
- test_regrtest: 1 min 6 sec

Ubuntu:

10 slowest tests:
- test_gdb: 3 min 18 sec
- test_multiprocessing_forkserver: 1 min 34 sec
- test_multiprocessing_fork: 1 min 15 sec
- test_venv: 1 min 1 sec
- test_compileall: 1 min
- test_cppext: 59.9 sec
- test_tarfile: 59.2 sec
- test_signal: 52.7 sec
- test_zipfile: 50.6 sec
- test_subprocess: 47.7 sec

Address Sanitizer:

10 slowest tests:
- test_subprocess: 2 min 8 sec
- test_venv: 2 min
- test_compileall: 1 min 49 sec
- test_gdb: 1 min 44 sec
- test_tarfile: 1 min 40 sec
- test_weakref: 1 min 18 sec
- test_pickle: 1 min 17 sec
- test_regrtest: 1 min 12 sec
- test_zipfile: 1 min 3 sec
- test_multiprocessing_main_handling: 1 min
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Aug 24, 2023

Copy link
Copy Markdown
Member Author

Shard tests i.e. run tests on two different jobs concurrently

test_asyncio was actually splitted into sub-modules like test_asyncio.test_subprocess and so in practice, "test_asyncio" is run in multiple processes in parallel, rather than a single process.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Aug 24, 2023

Copy link
Copy Markdown
Member Author

I merged PR #108396 to split multiprocessing tests into sub-tests, so they can be run in parallel.

I also wrote PR #108401 to split test_concurrent_futures into sub-tests (to run them in parallel).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Aug 24, 2023

Copy link
Copy Markdown
Member

See also #108421 which marks only the slowest tests, allowing faster tests to run.

The running time of test_concurrent_futures was reduced from 2 min 26 sec to 1 min 20 sec.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gpshead

gpshead commented Aug 24, 2023

Copy link
Copy Markdown
Member

With them split up I don't believe we should do use "requires cpu" for concurrent_futures or multiprocessing.

I rely on CI for testing of changes to things on most platforms. If github CI doesn't use -uall or -ucpu then I won't have a way to meaningfully test changes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Aug 24, 2023

Copy link
Copy Markdown
Member Author

@gpshead:

With them split up I don't believe we should do use "requires cpu" for concurrent_futures or multiprocessing.

Ok, I will continue in this way in this case.

@serhiy-storchaka's PR #108421is more narrow and so skips less tests.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner closed this Aug 24, 2023
@vstinner
vstinner deleted the requires_cpu2 branch August 24, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@vstinner @serhiy-storchaka @gpshead @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.