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

Weird int.__str__ behaviour inside sub-interpreters #117482

Open
mliezun opened this issue Apr 2, 2024 · 8 comments
Open

Weird int.__str__ behaviour inside sub-interpreters #117482

mliezun opened this issue Apr 2, 2024 · 8 comments
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes topic-subinterpreters type-bug An unexpected behavior, bug, or error

Comments

@mliezun
Copy link

mliezun commented Apr 2, 2024

Bug report

Bug description:

Hi Python maintainers!

I noticed something weird when using subinterpreters, while converting an Enum to string I get an unexpected result. This occurs in Python 3.12 and 3.13.

Here's an script to reproduce it:

import _xxsubinterpreters as interpreters


script = """from enum import _simple_enum, IntEnum

@_simple_enum(IntEnum)
class MyEnum:
    DATA = 1
    
print(str(MyEnum.DATA))
"""

exec(script)
# Output: 1

interp_id = interpreters.create(isolated=False)
interpreters.run_string(interp_id, script)
# Output: <MyEnum.DATA: 1>, Expected: 1

In all python versions previous to 3.12 this prints "1" two times, on newer versions I get <MyEnum.DATA: 1> when running inside a subinterpreter. For some reason, the __str__ function being used is different on new Python versions.

I also noticed that the function pointed by __str__ is different inside and outside the subinterpreter.

Outside:

...
print(MyEnum.DATA.__str__)
# Output: <method-wrapper '__repr__' of MyEnum object at 0x7f9a09a2e910>

Inside:

...
print(MyEnum.DATA.__str__)
# Output: <method-wrapper '__str__' of MyEnum object at 0x7f9a099a5e90>

NOTE: I'm creating subinterpreters passing the isolated=False flag, which uses the Legacy init config.
I first noticed the error on MacOS, then reproduced using Docker with various Python versions.

I hope this is enough to get to the source of the issue.

Appreciate all your work and effort on building Python, thank you!

CPython versions tested on:

3.8, 3.9, 3.10, 3.11, 3.12, 3.13

Operating systems tested on:

Linux, macOS

Linked PRs

@mliezun mliezun added the type-bug An unexpected behavior, bug, or error label Apr 2, 2024
@Eclips4
Copy link
Member

Eclips4 commented Apr 2, 2024

Bisected to de64e75

@Elchinchel
Copy link

I think i've got a shorter MRE, i believe this issue is not related to enums.

import _xxsubinterpreters as interpreters

script = """print(int.__str__)"""


exec(script)
# Output: <slot wrapper '__str__' of 'object' objects>

interp_id = interpreters.create()
interpreters.run_string(interp_id, script)
# Output: <slot wrapper '__str__' of 'int' objects>

@Eclips4
Copy link
Member

Eclips4 commented Apr 2, 2024

I think i've got a shorter MRE, i believe this issue is not related to enums.

import _xxsubinterpreters as interpreters

script = """print(int.__str__)"""


exec(script)
# Output: <slot wrapper '__str__' of 'object' objects>

interp_id = interpreters.create()
interpreters.run_string(interp_id, script)
# Output: <slot wrapper '__str__' of 'int' objects>

This sheds light on what is going on. Thanks, @Elchinchel!

@Eclips4 Eclips4 changed the title Weird Enum behaviour inside Subinterpreter Weird int.__str__ behaviour inside sub-interpreters Apr 2, 2024
@mliezun
Copy link
Author

mliezun commented Apr 3, 2024

Awesome! Thanks for looking into this @Eclips4 and @Elchinchel.

By running the simplified script, I noticed that other base types have the same issue: bool, float, complex.

The commit de64e75 you mentioned seems to change how the MRO works for base-types. I think that's probably related to the problem.

@ericsnowcurrently
Copy link
Member

FTR, #117660 (comment) provides further analysis.

@ericsnowcurrently ericsnowcurrently added 3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes labels Jul 10, 2024
ericsnowcurrently added a commit that referenced this issue Jul 11, 2024
When builtin static types are initialized for a subinterpreter, various "tp" slots have already been inherited (for the main interpreter).  This was interfering with the logic in add_operators() (in Objects/typeobject.c), causing a wrapper to get created when it shouldn't.  This change fixes that by preserving the original data from the static type struct and checking that.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 11, 2024
When builtin static types are initialized for a subinterpreter, various "tp" slots have already been inherited (for the main interpreter).  This was interfering with the logic in add_operators() (in Objects/typeobject.c), causing a wrapper to get created when it shouldn't.  This change fixes that by preserving the original data from the static type struct and checking that.
(cherry picked from commit 5250a03)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently added a commit that referenced this issue Jul 11, 2024
When builtin static types are initialized for a subinterpreter, various "tp" slots have already been inherited (for the main interpreter).  This was interfering with the logic in add_operators() (in Objects/typeobject.c), causing a wrapper to get created when it shouldn't.  This change fixes that by preserving the original data from the static type struct and checking that.

(cherry picked from commit 5250a03, AKA gh-121602)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this issue Jul 11, 2024
When builtin static types are initialized for a subinterpreter, various "tp" slots have already been inherited (for the main interpreter).  This was interfering with the logic in add_operators() (in Objects/typeobject.c), causing a wrapper to get created when it shouldn't.  This change fixes that by preserving the original data from the static type struct and checking that.
ericsnowcurrently added a commit that referenced this issue Jul 11, 2024
When builtin static types are initialized for a subinterpreter, various "tp" slots have already been inherited (for the main interpreter).  This was interfering with the logic in add_operators() (in Objects/typeobject.c), causing a wrapper to get created when it shouldn't.  This change fixes that by preserving the original data from the static type struct and checking that.

(cherry picked from commit 5250a03, AKA gh-121602)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
@AraHaan
Copy link
Contributor

AraHaan commented Jul 12, 2024

I think an alternative option is for tp_dict to be computed once and for those members of the type objects to remain untouched between main and sub-interpreters. As in once the main interpreter computes it and the inherited tp slots, it then uses those slots in any sub-interpreter and permanently solves this issue I think (I think this way it provides a performance boost for free in sub-interpreters on top of fixing this issue). Basically the entire PyTypeObject structure after the main interpreter properly loads and prepares it, the sub interpreters would be able to make use of the same pointers the main one used (by first copying the pointers to a new one for the subinterpreter using memcpy).

@ericsnowcurrently
Copy link
Member

@markshannon pointed out an alternate solution to the one I merged: in add_operators(), if *ptr isn't NULL then check if it is the same as the corresponding tp slot as the base (tp_base). If it is then act as though it were NULL.

There are good reasons to keep (and build on) the changes we've made already, for 3.13+. That isn't so much the case for 3.12 though. Thus, I'll probably update 3.12 to take the simpler approach.

estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
When builtin static types are initialized for a subinterpreter, various "tp" slots have already been inherited (for the main interpreter).  This was interfering with the logic in add_operators() (in Objects/typeobject.c), causing a wrapper to get created when it shouldn't.  This change fixes that by preserving the original data from the static type struct and checking that.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 23, 2024
… Static Builtin Types (pythongh-122192)

(cherry picked from commit 33d32fa)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently added a commit that referenced this issue Jul 23, 2024
…f Static Builtin Types (gh-122195)

(cherry picked from commit 33d32fa, AKA gh-122192)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this issue Jul 23, 2024
ericsnowcurrently added a commit that referenced this issue Jul 23, 2024
…f Static Builtin Types (gh-122197)

(cherry picked from commit 33d32fa, AKA gh-122192)
ericsnowcurrently added a commit that referenced this issue Jul 24, 2024
…h-121932)

In gh-121602, I applied a fix to a builtin types initialization bug.
That fix made sense in the context of some broader future changes,
but introduced a little bit of extra complexity.  For earlier versions
those future changes are not relevant; we can avoid the extra complexity.
Thus we can revert that earlier change and replace it with this one,
which is more focused and conceptually simpler.  This is essentially
the implementation of an idea that @markshannon pointed out to me.

Note that this change would be much smaller if we didn't have to deal
with repr compatibility for builtin types that explicitly inherit tp slots
(see expect_manually_inherited()).  The alternative is to stop
*explicitly* inheriting tp slots in static PyTypeObject values,
which is churn that we can do separately.
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this issue Jul 24, 2024
…ers (pythongh-121932)

In pythongh-121602, I applied a fix to a builtin types initialization bug.
That fix made sense in the context of some broader future changes,
but introduced a little bit of extra complexity.  For earlier versions
those future changes are not relevant; we can avoid the extra complexity.
Thus we can revert that earlier change and replace it with this one,
which is more focused and conceptually simpler.  This is essentially
the implementation of an idea that @markshannon pointed out to me.

Note that this change would be much smaller if we didn't have to deal
with repr compatibility for builtin types that explicitly inherit tp slots
(see expect_manually_inherited()).  The alternative is to stop
*explicitly* inheriting tp slots in static PyTypeObject values,
which is churn that we can do separately.

(cherry picked from commit 716c677, AKA pythongh-121932)
ericsnowcurrently added a commit that referenced this issue Jul 24, 2024
…h-122241)

In gh-121602, I applied a fix to a builtin types initialization bug.
That fix made sense in the context of some broader future changes,
but introduced a little bit of extra complexity.  For earlier versions
those future changes are not relevant; we can avoid the extra complexity.
Thus we can revert that earlier change and replace it with this one,
which is more focused and conceptually simpler.  This is essentially
the implementation of an idea that @markshannon pointed out to me.

Note that this change would be much smaller if we didn't have to deal
with repr compatibility for builtin types that explicitly inherit tp slots
(see expect_manually_inherited()).  The alternative is to stop
*explicitly* inheriting tp slots in static PyTypeObject values,
which is churn that we can do separately.

(cherry picked from commit 716c677, AKA gh-121932)
nohlson pushed a commit to nohlson/cpython that referenced this issue Jul 24, 2024
nohlson pushed a commit to nohlson/cpython that referenced this issue Jul 24, 2024
ericsnowcurrently added a commit that referenced this issue Jul 29, 2024
The tests were only checking cases where the slot wrapper was present in the initial case.  They were missing when the slot wrapper was added in the additional initializations.  This fixes that.
ericsnowcurrently added a commit that referenced this issue Jul 29, 2024
The tests were only checking cases where the slot wrapper was present in the initial case.  They were missing when the slot wrapper was added in the additional initializations.  This fixes that.

(cherry-picked from commit 490e0ad, AKA gh-122248)
ericsnowcurrently added a commit that referenced this issue Jul 29, 2024
The tests were only checking cases where the slot wrapper was present in the initial case.  They were missing when the slot wrapper was added in the additional initializations.  This fixes that.

(cherry-picked from commit 490e0ad, AKA gh-122248)
ericsnowcurrently added a commit that referenced this issue Aug 12, 2024
…gh-122867)

There were a still a number of gaps in the tests, including not looking
at all the builtin types and not checking wrappers in subinterpreters
that weren't in the main interpreter. This fixes all that.

I considered incorporating the names of the PyTypeObject fields
(a la gh-122866), but figured doing so doesn't add much value.
blhsing pushed a commit to blhsing/cpython that referenced this issue Aug 22, 2024
…orough (pythongh-122867)

There were a still a number of gaps in the tests, including not looking
at all the builtin types and not checking wrappers in subinterpreters
that weren't in the main interpreter. This fixes all that.

I considered incorporating the names of the PyTypeObject fields
(a la pythongh-122866), but figured doing so doesn't add much value.
encukou pushed a commit that referenced this issue Sep 9, 2024
In gh-121602, I applied a fix to a builtin types initialization bug.
That fix made sense in the context of some broader future changes,
but introduced a little bit of extra complexity. That fix has turned
out to be incomplete for some of the builtin types we haven't
been testing. I found that out while improving the tests.

A while back, @markshannon suggested a simpler fix that doesn't
have that problem, which I've already applied to 3.12 and 3.13.
I'm switching to that here. Given the potential long-term
benefits of the more complex (but still incomplete) approach,
I'll circle back to it in the future, particularly after I've improved
the tests so no corner cases slip through the cracks.

(This is effectively a "forward-port" of 716c677 from 3.13.)
@encukou
Copy link
Member

encukou commented Sep 9, 2024

Note that current PRs are closed, the issue should stay open. @ericsnowcurrently said in #122865:

I'll circle back to it in the future, particularly after I've improved
the tests so no corner cases slip through the cracks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes topic-subinterpreters type-bug An unexpected behavior, bug, or error
Projects
Status: Todo
Development

No branches or pull requests

6 participants