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

Mark slow test methods with @requires_resource('cpu') #108416

Closed
serhiy-storchaka opened this issue Aug 24, 2023 · 7 comments
Closed

Mark slow test methods with @requires_resource('cpu') #108416

serhiy-storchaka opened this issue Aug 24, 2023 · 7 comments
Assignees
Labels
tests Tests in the Lib/test dir

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 24, 2023

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Proposal:

The proposed PR marks all test methods which have duration longer than 3 seconds with @test.support.requires_resource('cpu') decorator.

The purpose is to reduce manual testing time. It happens that all tests in a file are ran in fraction of a second, but few tests take a long time to run. When you work on some large feature or bugfix you need to run corresponding tests multiple times. You can exclude the slowest tests manually, but you should know what of them are culprits. When they are marked as CPU-hungry, you can just not enable the "cpu" resource.

For example, all test_math takes over 1.5 minutes to finish. But when exclude test_sumprod_stress, it takes only 3 seconds.

Linked PRs

@serhiy-storchaka serhiy-storchaka added the tests Tests in the Lib/test dir label Aug 24, 2023
@serhiy-storchaka serhiy-storchaka self-assigned this Aug 24, 2023
@sobolevn
Copy link
Member

Isn't it the same as #108388 ?

@serhiy-storchaka
Copy link
Member Author

No, they are different. The purpose is different, this issue is about speeding up manual testing, although it can help in CI testing too. I only mark the slowest methods, allowing fast tests to run. #108388 is much larger and more complex issue, this issue only intersects with a small part of it..

@gpshead
Copy link
Member

gpshead commented Aug 24, 2023

This doesn't feel right. Tagging these as 'cpu' is usually untrue. Many of these tests are not resource intensive for anything but 'wall clock time'.

Critically, it would means we have no easy reliable way of running this vast swath of our testsuite across multiple platforms when doing development if our Github CI does not use whatever resource is being applied so that the tests become skipped by default. If a dev has to do something special to identify and make tests potentially relevant change run across all platforms, they simply not going to get that determination right or remember a large portion of the time and the tests will not be run until after merge when it is too late. Allowing regressions to be merged is a net negative.

Tangentially related: We should probably have all github actions CI configurations in release branches set to use -uall no matter what.

@gpshead

This comment was marked as resolved.

@gpshead
Copy link
Member

gpshead commented Aug 25, 2023

I think we want a new resource class for this purpose when latency is the main reason it is being disabled rather than actual CPU consumption. I suggest perhaps calling it 'walltime'?

We could make a new -u category setting for github CI use called -ufast and configure specific github actions we want to respond faster to specify that. We could then control which other settings are turned on or off via that within regrtest itself. Leaving walltime enabled by default so that devs run those tests themselves by default. It'd be good to have at least one non-required GH actions CI check not using the "fast" run, or even doing a "-uall,-network,-largefile" run, as that still provides visibility on the PR.

Even better if it is possible for the github merge queue automerge to wait for that not otherwise required CI check even though the merge button blocker does not? I don't know if github allows that level of config... @ambv ?

(brainstorming)

@serhiy-storchaka
Copy link
Member Author

Re-did using time.process_time() instead of time.perf_counter(), and then using sum(os.times()[:4]), because some tests run CPU-heavy subprocesses. Now it includes about 46 tests.

I prepared also a separate patch for marking long running tests which do not spend much CPU time, but simply sleep, with a new resource "walltime".

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Aug 25, 2023
serhiy-storchaka added a commit that referenced this issue Sep 2, 2023
…108421)

Only mark tests which spend significant system or user time,
by itself or in subprocesses.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 2, 2023
pythonGH-108421)

Only mark tests which spend significant system or user time,
by itself or in subprocesses.
(cherry picked from commit f3ba0a7)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Sep 2, 2023
…e('cpu') (pythonGH-108421)

Only mark tests which spend significant system or user time,
by itself or in subprocesses..
(cherry picked from commit f3ba0a7)

Co-authored-by: Serhiy Storchaka <[email protected]>
Sep 2, 2023
…') (GH-108421) (#108798)

gh-108416: Mark slow test methods with @requires_resource('cpu') (GH-108421)

Only mark tests which spend significant system or user time,
by itself or in subprocesses.
(cherry picked from commit f3ba0a7)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit that referenced this issue Sep 3, 2023
…') (GH-108421) (GH-108799)

Only mark tests which spend significant system or user time,
by itself or in subprocesses.
(cherry picked from commit f3ba0a7)
@serhiy-storchaka
Copy link
Member Author

See also #108828 which will help to find all tests which can be skipped for particular reason.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 5, 2023
…es_resource('walltime') (pythonGH-108480)

(cherry picked from commit 1e0d627)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Sep 5, 2023
… requires_resource('walltime') (pythonGH-108480).

(cherry picked from commit 1e0d627)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit that referenced this issue Sep 5, 2023
…res_resource('walltime') (GH-108480) (GH-108924)

(cherry picked from commit 1e0d627)
vstinner pushed a commit to vstinner/cpython that referenced this issue Sep 5, 2023
Sep 8, 2023
* gh-108834: regrtest reruns failed tests in subprocesses (#108839)

When using --rerun option, regrtest now re-runs failed tests
in verbose mode in fresh worker processes to have more
deterministic behavior. So it can write its final report even
if a test killed a worker progress.

Add --fail-rerun option to regrtest: exit with non-zero exit code
if a test failed pass passed when re-run in verbose mode (in a
fresh process). That's now more useful since tests can pass
when re-run in a fresh worker progress, whereas they failed
when run after other tests when tests are run sequentially.

Rename --verbose2 option (-w) to --rerun. Keep --verbose2 as a
deprecated alias.

Changes:

* Fix and enhance statistics in regrtest summary. Add "(filtered)"
  when --match and/or --ignore options are used.
* Add RunTests class.
* Add TestResult.get_rerun_match_tests() method
* Rewrite code to serialize/deserialize worker arguments as JSON
  using a new WorkerJob class.
* Fix stats when a test is run with --forever --rerun.
* If failed test names cannot be parsed, log a warning and don't
  filter tests.
* test_regrtest.test_rerun_success() now uses a marker file, since
  the test is re-run in a separated process.
* Add tests on normalize_test_name() function.
* Add test_success() and test_skip() tests to test_regrtest.

(cherry picked from commit 31c2945)

* gh-108834: regrtest --fail-rerun exits with code 5 (#108896)

When the --fail-rerun option is used and a test fails and then pass,
regrtest now uses exit code 5 ("rerun) instead of 2 ("bad test").

(cherry picked from commit 1170d5a)

* gh-108416: Mark slow but not CPU bound test methods with requires_resource('walltime') (GH-108480)

(cherry picked from commit 1e0d627)

* Manually sync Lib/test/libregrtest/ from main

---------

Co-authored-by: Serhiy Storchaka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

No branches or pull requests

3 participants