8000 Test failure in `tests/test_optimizer.py` · Issue #115058 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

Test failure in tests/test_optimizer.py #115058

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.

Sign up for GitHub

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

Jump to bottom
Closed
freakboy3742 opened this issue Feb 6, 2024 · 14 comments
Closed

Test failure in tests/test_optimizer.py #115058

freakboy3742 opened this issue Feb 6, 2024 · 14 comments
Labels
3.13 bugs and security fixes tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@freakboy3742
Copy link
Contributor
freakboy3742 commented Feb 6, 2024 •
edited by bedevere-app bot
Loading

Bug report

Bug description:

As of 01dceba, I'm seeing a test failure on macOS and iOS in the tests/test_optimizer.py test case.

If you run the test by itself, (python -m test tests/test_optimizer.py) it passes.

However, if you run the entire test suite in default (alphabetical) order, it fails:

test test_optimizer failed -- Traceback (most recent call last):
  File "/Users/rkm/projects/python/host/lib/python3.13/test/test_optimizer.py", line 52, in test_builtin_dict
    self.assertEqual(
    ~~~~~~~~~~~~~~~~^
        orig_counter + 1,
        ^^^^^^^^^^^^^^^^^
        _testinternalcapi.get_rare_event_counters()["builtin_dict"]
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
AssertionError: 256 != 255

test_optimizer failed (1 failure)

== Tests result: FAILURE ==

I've been able to narrow down a minimal reproduction case if you run the following sequence of tests:

python -m test test_fileinput test_funcattrs test_functools test_generators test_genexps test_getopt test_gettext test_optimizer

If you remove any one of the tests before test_optimizer, the test passes.

I'm observing the failure on both x86_64 (Ventura 13.5.2) and M1 (Ventura 13.6.2) hardware.

CPython versions tested on:

CPython main branch

Operating systems tested on:

macOS, Other

Linked PRs
@freakboy3742 freakboy3742 added type-bug An unexpected behavior, bug, or error OS-mac 3.13 bugs and security fixes OS-ios labels Feb 6, 2024
@terryjreedy terryjreedy added tests Tests in the Lib/test dir and removed OS-mac OS-ios labels Feb 6, 2024
@terryjreedy
Copy link
Member

Same exact failure on Windows. Hence remove OS specific labels.

@Eclips4
Copy link
Member
Eclips4 commented Feb 6, 2024

cc @mdboom

@boshea93
Copy link
boshea93 commented Feb 6, 2024 •
edited
Loading

In line 46 of test_optimizer.test_builtin_dict, orig_counter is already set to 255. After line 51, _testinternalcapi.get_rare_event_counters()["builtin_dict"] doesn't increment past 255. Is it possible that the count is tied to some 256 bit size limit?
image

@Eclips4
Copy link
Member
Eclips4 commented Feb 6, 2024 •
edited
Loading

In line 46 of test_optimizer.test_builtin_dict, orig_counter is already set to 255. After line 51, _testinternalcapi.get_rare_event_counters()["builtin_dict"] doesn't increment past 255. Is it possible that the count is tied to some 256 bit size limit?

Hello!
Indeed,
interp->rare_events.builtin_dict has type uint8_t.
But I'm wonder why it's not overflowed and not become 0

@Eclips4
Copy link
Member
Eclips4 commented Feb 6, 2024

I've check it manually, and if I manually increase the counter (when it's equal to 255) it's become a zero.
So, there are two problems:

  1. The counter is not increasing somewhere
  2. Even the counter get increased, it's become a zero (should be a 256)

@Eclips4
Copy link
Member
Eclips4 commented Feb 6, 2024 •
edited
Loading

I've check it manually, and if I manually increase the counter (when it's equal to 255) it's become a zero. So, there are two problems:

  1. The counter is not increasing somewhere
  2. Even the counter get increased, it's become a zero (should be a 256)

Ok, now I'm understand:

#define RARE_EVENT_INTERP_INC(interp, name) \
    do { \
        /* saturating add */ \
        if (interp->rare_events.name < UINT8_MAX) interp->rare_events.name++; \
        RARE_EVENT_STAT_INC(name); \
    } while (0); \

This macros doesn't allow the counter to increase above UINT8_MAX.
If I rewrite macros using UINT16_MAX and change the type of interp->rare_events.builtin_dict from uint8_t to uint16_t the issue is going to disappear:

./python.exe -m test test_fileinput test_funcattrs test_functools test_generators test_genexps test_getopt test_gettext test_optimizer
Using random seed: 3588920112
0:00:00 load avg: 52.24 Run 8 tests sequentially
0:00:00 load avg: 52.24 [1/8] test_fileinput
0:00:00 load avg: 52.24 [2/8] test_funcattrs
0:00:00 load avg: 52.24 [3/8] test_functools
0:00:00 load avg: 52.24 [4/8] test_generators
0:00:00 load avg: 52.24 [5/8] test_genexps
0:00:00 load avg: 52.24 [6/8] test_getopt
0:00:00 load avg: 52.24 [7/8] test_gettext
0:00:00 load avg: 52.24 [8/8] test_optimizer

== Tests result: SUCCESS ==

All 8 tests OK.

Total duration: 898 ms
Total tests: run=458
Total test files: run=8/8
Result: SUCCESS

So, probably it's enough to just change the type of builtin_dict(and other fields) for a larger type?

Eclips4 added a commit to Eclips4/cpython that referenced this issue Feb 7, 2024
@Eclips4
pythongh-115058: Replace types with a larger types
c7052a9
@bedevere-app bedevere-app bot mentioned this issue Feb 7, 2024
Merged
@mdboom
Copy link
Contributor
mdboom commented Feb 9, 2024

I'm bringing @markshannon into the discussion here, because it's his design.

I'm not sure we want to just increase the sizes here, certainly not on all of the fields. I think the better solution is probably to "reset" the builtins counter to 0 at some point after most startup has happened. Or maybe modify the test to reset the counter before starting -- I think these high numbers are the result of the test suite being unusual, and doesn't reflect the rareness of builtin modification one would expect in normal code.

@markshannon
Copy link
Member

Or change the test from counter == orig_counter + 1 to counter == orig_counter + 1 or counter == 255?

@mdboom
Copy link
Contributor
mdboom commented Feb 9, 2024

Or change the test from counter == orig_counter + 1 to counter == orig_counter + 1 or counter == 255?

I worry that could hide a bona fide bug.

I think the better solution is to add a function in testinternalcapi to reset the counter and then run the test as-is.

@Eclips4: Do you want to modify your existing PR to do that, or should I take it? (I'm happy either way).

@markshannon
Copy link
Member

Reseting the counter to zero seems better for testing, but it is possible an optimization would rely on the count being correct.
As long as the reset is hidden in _testinternalcapi I guess it would be fine

@boshea93
Copy link
boshea93 commented Feb 9, 2024

Reseting the counter to zero seems better for testing, but it is possible an optimization would rely on the count being correct. As long as the reset is hidden in _testinternalcapi I guess it would be fine

Hi @markshannon and @mdboom! I apologize if this is a silly question. I was curious about when the counter reset would occur? Would it be after the tests from one test module have finished running, the counter hits 255, or when another condition is met?

@mdboom
Copy link
Contributor
mdboom commented Feb 9, 2024

My thought is that there would be a new function added to _testinternalcapi which would reset the counter. Then this function would be called at the beginning of the test_builtin_dict test.

@Eclips4
Copy link
Member
Eclips4 commented Feb 9, 2024

Or change the test from counter == orig_counter + 1 to counter == orig_counter + 1 or counter == 255?

I worry that could hide a bona fide bug.

I think the better solution is to add a function in testinternalcapi to reset the counter and then run the test as-is.

@Eclips4: Do you want to modify your existing PR to do that, or should I take it? (I'm happy either way).

I'm agree that this is the better solution. I'm going to update my PR

@Eclips4
Copy link
Member
Eclips4 commented Feb 9, 2024

My thought is that there would be a new function added to _testinternalcapi which would reset the counter. Then this function would be called at the beginning of the test_builtin_dict test.

I have updated the PR, please check it :)

markshannon pushed a commit that referenced this issue Feb 12, 2024
@Eclips4
gh-115058: Add reset_rare_event_counters function in `_testintern…
93ac78a 
…alcapi` (GH-115128)
@Eclips4 Eclips4 closed this as completed Feb 12, 2024
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this issue Feb 14, 2024
@Eclips4 @fsc-eriker
pythongh-115058: Add reset_rare_event_counters function in `_test…
ca5fdf0 
…internalcapi` (pythonGH-115128)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.13 bugs and security fixes tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@freakboy3742 @mdboom @markshannon @terryjreedy @boshea93 @Eclips4
0