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

test_unicode always fails during a PGO-optimised build (because it does not exist) #110276

Closed
AlexWaygood opened this issue Oct 3, 2023 · 8 comments
Assignees
Labels
build The build process and cross-build tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 3, 2023

Bug report

Bug description:

As part of a PGO-optimised build, a selection of tests are run in order for the PGO build to take place (if I understand correctly, these tests are used to gather data on which functions in Python are performance-critical and should be optimised). Exactly which tests are run is hardcoded here:

# Set of tests run by default if --pgo is specified. The tests below were
# chosen based on the following criteria: either they exercise a commonly used
# C extension module or type, or they run some relatively typical Python code.
# Long running tests should be avoided because the PGO instrumented executable
# runs slowly.
PGO_TESTS = [
'test_array',
'test_base64',
'test_binascii',
'test_binop',
'test_bisect',
'test_bytes',
'test_bz2',
'test_cmath',
'test_codecs',
'test_collections',
'test_complex',
'test_dataclasses',
'test_datetime',
'test_decimal',
'test_difflib',
'test_embed',
'test_float',
'test_fstring',
'test_functools',
'test_generators',
'test_hashlib',
'test_heapq',
'test_int',
'test_itertools',
'test_json',
'test_long',
'test_lzma',
'test_math',
'test_memoryview',
'test_operator',
'test_ordered_dict',
'test_patma',
'test_pickle',
'test_pprint',
'test_re',
'test_set',
'test_sqlite3',
'test_statistics',
'test_struct',
'test_tabnanny',
'test_time',
'test_unicode',
'test_xml_etree',
'test_xml_etree_c',
]

test_unicode is specified as one of the tests to be run as part of this selection. However, test_unicode always fails during a PGO build. Why? Because it was renamed as test_str in #13172 (by @asqui). This hasn't been noticed until now because the PGO build is still marked as a "success" even if some tests fail during the analysis phase.

The fix for the immediate issue is simple: don't try to run test_unicode as part of the PGO-optimised build (it doesn't exist anymore); run test_str instead. Longer term, though, we might want to look at making the PGO build fail if it tries to run nonexistent tests. It's probably correct for the build to continue even if some tests fail during the analysis phase, but perhaps we could make an exception for this specific kind of failure (attempting to run nonexistent tests).

Cc. @hugovk as the primary reviewer for #13172, and @vstinner as a libregrtest expert.

CPython versions tested on:

CPython main branch

Operating systems tested on:

Windows

Linked PRs

@AlexWaygood AlexWaygood added type-bug An unexpected behavior, bug, or error tests Tests in the Lib/test dir build The build process and cross-build labels Oct 3, 2023
@AlexWaygood AlexWaygood self-assigned this Oct 3, 2023
AlexWaygood added a commit to AlexWaygood/cpython that referenced this issue Oct 3, 2023
AlexWaygood added a commit that referenced this issue Oct 3, 2023
@vstinner
Copy link
Member

vstinner commented Oct 3, 2023

I don't understand this issue. We have many PGO buildbots.

How is it possible that a test was removed and nothing broke?

If I replace PGO_TESTS with PGO_TESTS=['test_xxx'], tests fail as expected:

$ ./python -m test --pgo; echo $?
0:00:00 load avg: 1.80 Run 1 test sequentially
0:00:00 load avg: 1.80 [1/1] test_xxx
test_xxx failed (uncaught exception)

Total duration: 54 ms
Total tests: run=0
Total test files: run=1/1 failed=1
Result: FAILURE
2

Maybe the problem is that Makefile.pre.in ignores failures :-(

        $(LLVM_PROF_FILE) $(RUNSHARED) ./$(BUILDPYTHON) $(PROFILE_TASK) || true
(...)
        # Run instrumented binaries to collect data.
        $(RUNSHARED) ./$(BUILDPYTHON) $(PROFILE_TASK) || true

@corona10: Any idea why we ignore failures?

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Oct 3, 2023

FWIW, the failure wasn't "just on my machine": you can see the "test_unicode failed (uncaught exception)" message in the logs for our PGO buildbots too. Example: https://buildbot.python.org/all/#/builders/249/builds/6518/steps/3/logs/stdio.

Maybe the problem is that Makefile.pre.in ignores failures :-(

Hmm, I'm on Windows, and the test failure doesn't cause a PGO build (which doesn't use the Makefile on Windows) to fail on my machine either. The test fails as part of the build, the failure is reported and logged to the terminal, but the build then continues and is marked as a success.

@vstinner
Copy link
Member

vstinner commented Oct 3, 2023

FWIW, the failure wasn't "just on my machine": you can see the "test_unicode failed (uncaught exception)" message in the logs for our PGO buildbots too.

A log is not really what I expect when I say "failure". I expected a red buildbot :-) Otherwise, it's too easy to miss.

@AlexWaygood
Copy link
Member Author

FWIW, the failure wasn't "just on my machine": you can see the "test_unicode failed (uncaught exception)" message in the logs for our PGO buildbots too.

A log is not really what I expect when I say "failure". I expected a red buildbot :-) Otherwise, it's too easy to miss.

Oh, we agree! There's definitely a problem here that this issue went unseen for so long! :D

This is the reason why I haven't closed the issue yet, even though #110277 has now been merged, fixing the immediate problem.

@vstinner
Copy link
Member

vstinner commented Oct 3, 2023

I wrote PR #110295 to no longer ignore PROFILE_TASK failure silently.

@vstinner
Copy link
Member

vstinner commented Oct 3, 2023

My commit 6ab6040 no longer ignores PROFILE_TASK failure silently.

@AlexWaygood
Copy link
Member Author

My commit 6ab6040 no longer ignores PROFILE_TASK failure silently.

It sounds like there is little appetite among the core devs who maintain the Windows build for an equivalent PCBuild fix, so I'll close this out now. Thanks @vstinner!

@vstinner
Copy link
Member

vstinner commented Oct 4, 2023

PGO builds are tested daily on Unix by buildbots. So now we should get any similar issue get reported way earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants