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

PEP 630: add disclaimers re. heap types & conversion, add PyType_GetModuleByDef #2319

Merged
merged 2 commits into from
Feb 14, 2022

Conversation

encukou
Copy link
Member

@encukou encukou commented Feb 11, 2022

  • Make it clear that it's not necessary to convert all static types to heap ones

  • Make it clear that this is a generic guide, and doesn't cover stdlib-specific issues

  • Add PyType_GetModuleByDef, closing the #1 open issue

  • Add an open isssue re. converting “losslessly” to heap types

I'd like to apologize to everyone who was misled by the previous version of this PEP.
When I wrote it, I thought heap types were definitely the correct direction, and wrote the text with that in mind. But it turns out that this PEP could be used as the only rationale for converting to heap types. That wasn't my intention.
I still think heap types are good, especially in third-party modules, all else being equal. But in the stdlib, there should be a specific reason for each heap type conversion.
I hope that in many cases, good reasons can be found in this PEP. But it doesn't cover all cases.

FWIW, this part is unchanged:

As with any Informational PEP, this text does not necessarily represent
a Python community consensus or recommendation.

cc @vstinner @erlend-aasland @ericsnowcurrently @ncoghlan

pep-0630.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks good to me. I left some comments.

A couple of questions:

  • How much can you alter an already accepted PEP? See Brett's email to python-dev
  • Does the SC need to "re-approve" such edits? (IMO, getting a review from at least one SC member should be required for already approved PEPs.)

pep-0630.rst Outdated Show resolved Hide resolved
For new modules, using heap types by default is a good rule of thumb.

Existing classes can be converted to heap ones, but note that
the heap type API was not designed for “lossless” conversion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Lossless" is a little vague IMO. Could you reword this with a clearer language?

pep-0630.rst Outdated

Existing classes can be converted to heap ones, but note that
the heap type API was not designed for “lossless” conversion
from static types. Unlike static types, heap type objects are mutable.
Copy link
Contributor

@erlend-aasland erlend-aasland Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heap type objects are mutable by default. With the current wording it sounds like they are always mutable; that is not true. I strongly suggest you reconsider your wording in order to avoid misunderstandings. Explicit is better than implicit.

Suggested change
from static types. Unlike static types, heap type objects are mutable.
from static types. Unlike static types, heap type objects are mutable by default.
Heap types can be made immutable by using the flag ``Py_TPFLAGS_IMMUTABLETYPE``.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Somehow I forgot about that 3.10 addition of Py_TPFLAGS_IMMUTABLETYPE.
Adding "by default" makes the PEP technically correct, though it'll be a while before common libraries can drop 3.9 and use immutable heap types.
Luckily, having types immutable is a lot more important in stdlib than elsewhere.

the heap type API was not designed for “lossless” conversion
from static types. Unlike static types, heap type objects are mutable.
Also, when rewriting the class definition in a new API,
you are likely to unintentionally change a few details (e.g. pickle-ability
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reconsider this wording. Take care not to IMO sounds nicer than You are likely to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is the pickle issue still an issue? Wasn't that fixed by Serhiy long ago? The bpo issue that mentioned this issue is closed, marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pickle issue was closed, because there's now a mechanism to use if preserving pickle-ability is important for you. In the stdlib, it pretty much always is important, but some third-party libraries won't care.
Same for mutability.
OTOH, there are unexplored issues that stdlib doesn't run into but other libraries do, e.g. ones involving complex inheritance chains or metaclasses.

To keep the PEP general, I don't want to include the specific fixes here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get back to the original comment, I think changing the behavior is a fact of life rather than something to avoid. At least right now.
(It's different in the stdlib.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTOH, there are unexplored issues that stdlib doesn't run into but other libraries do, e.g. ones involving complex inheritance chains or metaclasses.

To keep the PEP general, I don't want to include the specific fixes here.

Sounds good to me.

pep-0630.rst Outdated Show resolved Hide resolved
pep-0630.rst Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

There was a fix for the serialization (pickle):

@encukou
Copy link
Member Author

encukou commented Feb 14, 2022

Does the SC need to "re-approve" such edits?

This PEP is not accepted. The SC did not review it. It's a guide for intended usage of features from a bunch of already approved PEPs, and a roadmap for future ones.
Again, the PEP itself includes a quote from PEP 1:

As with any Informational PEP, this text does not necessarily represent a Python community consensus or recommendation.

@erlend-aasland
Copy link
Contributor

This PEP is not accepted. The SC did not review it. It's a guide for intended usage of features from a bunch of already approved PEPs, and a roadmap for future ones. Again, the PEP itself includes a quote from PEP 1:

Thanks for the heads up, Petr. I was not aware of that fact.

pep-0630.rst Outdated Show resolved Hide resolved
pep-0630.rst Outdated Show resolved Hide resolved
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

@encukou
Copy link
Member Author

encukou commented Feb 14, 2022

Thanks for the review!

I was not aware of that fact.

That explains so much! I'm glad we found a source of some of the misunderstandings.

@encukou encukou merged commit bdc0e44 into python:main Feb 14, 2022
@encukou encukou deleted the pep-isolating branch February 14, 2022 15:30
debonte pushed a commit to debonte/peps that referenced this pull request Feb 14, 2022
…oduleByDef (pythonGH-2319)

* PEP 630: add disclaimers re. heap types & conversion, and PyType_GetModuleByDef

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants