PyCode_New API change breaks backwards compatibility policy

[ncoghlan]

BPO	37221
Nosy	@nascheme, @ncoghlan, @scoder, @vstinner, @encukou, @ambv, @serhiy-storchaka, @jdemeyer, @pablogsal, @miss-islington
PRs	bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper #13959 [3.8] bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper (GH-13959) #14505 bpo-37221: PyCode_New() didn't change in Python 3.8 #23595 [3.8] bpo-37221: PyCode_New() didn't change in Python 3.8 (GH-23595) #23599 [3.9] bpo-37221: PyCode_New() didn't change in Python 3.8 (GH-23595) #23600

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-07-07.09:10:14.565>
created_at = <Date 2019-06-10.23:24:27.388>
labels = ['build', '3.8', 'release-blocker']
title = 'PyCode_New API change breaks backwards compatibility policy'
updated_at = <Date 2020-12-01.15:55:03.350>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2020-12-01.15:55:03.350>
actor = 'miss-islington'
assignee = 'none'
closed = True
closed_date = <Date 2019-07-07.09:10:14.565>
closer = 'scoder'
components = []
creation = <Date 2019-06-10.23:24:27.388>
creator = 'ncoghlan'
dependencies = []
files = []
hgrepos = []
issue_num = 37221
keywords = ['patch', '3.8regression']
message_count = 35.0
messages = ['345153', '345154', '345168', '345172', '345196', '345229', '345230', '345271', '345272', '345279', '345281', '345283', '345284', '345305', '345311', '345312', '345313', '345318', '345334', '345338', '346224', '346389', '346405', '346427', '346993', '346996', '346997', '347004', '347054', '347414', '347415', '347468', '382261', '382264', '382267']
nosy_count = 10.0
nosy_names = ['nascheme', 'ncoghlan', 'scoder', 'vstinner', 'petr.viktorin', 'lukasz.langa', 'serhiy.storchaka', 'jdemeyer', 'pablogsal', 'miss-islington']
pr_nums = ['13959', '14505', '23595', '23599', '23600']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue37221'
versions = ['Python 3.8']

[ncoghlan]

The Porting section of the What's New guide is for changes where the old behaviour was at best arguably correct, but it's still possible someone was relying on it behaving exactly the way it used to.

It isn't for us to say "We broke all extensions that use this existing public C API by adding a new parameter to its signature".

For 3.8b2, the function with the extra parameter should be renamed to PyCode_NewEx, and a PyCode_New compatibility wrapper added that calls it with the extra parameter set to zero.

[vstinner]

PyCode_New() and types.CodeType constructor are actively discussed:

• https://mail.python.org/archives/list/python-dev@python.org/thread/VXDPH2TUAHNPT5K6HBUIV6VASBCKKY2K/
• bpo-36896
• bpo-36886

[pablogsal]

Thanks, Nick for opening this issue. If everyone agrees this is the best path forward I can make a PR. Take into account that doing such rename will break again the projects that have adapted already (primarily only Cython). I would like to give some context around the change itself.

• Last time the function was changed in commit 4f72a78 (for adding keyword-only parameters) a wrapper was not created or a new compatibility layer was not created. This happened as well in the past when the function slowly growed from 3 parameters in 1990 to 16 parameters currently. If we decide to provide the wrapper, this would be the first time we do this for changes in this function.

• In the past, backwards incompatible changes that came from syntax changes were reported in the What's New. For example, in Python3.6 we added "async and await names are now reserved keywords. Code using these names as identifiers will now raise a SyntaxError" in the porting session and one could argue that that broke more code than this change. I am not saying this to justify breaking changes, just to give more context of somehow similar situations on how we report changes that we knew that were backwards incompatible due to new features.

• A quick (and I acknowledge that is partial and incomplete) search in GitHub about usages of PyCode_New excluding interpreter files to avoid forks and the like, points mainly at Cython (and Pyrex forks). Cython already adapted to this change, so changing it back will break Cython and all other possible extensions that already adapted after the last alpha.

• In the email thread https://mail.python.org/archives/list/python-dev@python.org/thread/VXDPH2TUAHNPT5K6HBUIV6VASBCKKY2K/, Stefan Behnel (who is also a Cython maintainer) said in relation to adding PyCode_NewEx:

It's not a commonly used function, and it's easy for C code to adapt. I
don't think it's worth adding a new function to the C-API here, compared to
just changing the signature. Very few users would benefit, at the cost of
added complexity.

[pablogsal]

I have created PR13959 in case we decide to go with the PyCode_NewEx path.

[serhiy-storchaka]

Seems it was changed last time many years ago and was stable in Python 3.

[jdemeyer]

Take into account that doing such rename will break again the projects that have adapted already (primarily only Cython).

+1. You already broke backwards compatibility once in beta1, no need to do it again in beta2.

[pablogsal]

You already broke backwards compatibility once in beta1

There is one other thing to consider, that is projects with pinned dependencies will have to upgrade to the last Cython and update their generated sources if we don't do the renaming. I don't know how often this happens (for example, Python3.7 changed the result of PyUnicode_AsUTF8AndSize() and PyUnicode_AsUTF8() to type const char * rather of char * and that is backward incompatible if you compile with a C++ compiler or with -Werror) but certainly is worth to consider.

Also, take into account that beta period is precisely the moment to fix these kinds of problems (although I apologize for any inconvenience this may have caused).

[vstinner]

I would prefer to revert PyCode_New() API (to Python 3.7 and older API), and add a *new* function for positional-only arguments.

I have created PR13959 in case we decide to go with the PyCode_NewEx path.

I dislike "Ex" suffix. What will be next name? Ex2? NewEx?

I prefer "With" naming.

I suggest: PyCode_NewWithPosArgs(). IMHO it's way more explicit when you opt-in for this new function ;-)

--

PyCode_New() was broken 8 times in the history of Python :-)
https://bugs.python.org/issue37032#msg343377

It seems like more people are unhappy with this backward incompatible change, because Python popularity is still growing. It's a good sign of the health of time :-)

--

+1. You already broke backwards compatibility once in beta1, no need to do it again in beta2.

It's not this easy.

This issue mostly impact projects using Cython. For practical reasons, projects include C files generated by Cython (to avoid Cython dependency to install the project). Cython 0.29.8 is the first version supporting Python 3.8 (new PyCode_New API) was only released two weeks ago.

Right now, I guess that almost all, if not all, tarballs on PyPI on projects using Cython still include C code only compatible with Python 3.7 (old PyCode_New API).

There are 183k projets on PyPI. I would prefer to not have to have to manually regenerate the C code they include to support the new PyCode_New() API.

We are still at beta stage. The role of beta releases is to detect backward incompatible changes like that.

I'm in favor of reverting PyCode_New() API and add a new function for the very few people who care about building manually a code object with positional-only arguments.

It's just a practical move to not break projects on PyPI.

Please, synchronize with Cython to make sure that we can get a Cython release soon after beta2 with will emit code working on all Python version. Maybe the workaround for Python 3.8 alpha1 .. 3.8 beta1 can be simply removed from Cython?

[vstinner]

It seems like very few pepople are testing Python 3.8 so far. I'm making this assumption because many projects using Cython cannot be installed on Python 3.8 and I didn't see much complain so far.

My Python Maintenance team at Red Hat is working on fixing all the stuff for Python 3.8, and we are the most exposed by such backward incompatible change. FYI we modified a few Fedora packages to explicitly run Cython as part of the build to ensure that C files are always regenerated with Cython. So if PyCode_New() API change again in beta2 but Cython takes that into account, Fedora Rawhide will be unaffected.

Note: we are not sure yet if Fedora 31 will be able to switch to Python 3.8 by default, the build of many package is still failing, for various reasons (sadly, this change is far from being the only reason!).

More info:
https://twitter.com/VictorStinner/status/1138067403341012997

[ncoghlan]

The key problem isn't Cython itself, the problem is that Cython generated libraries can't be rebuilt for 3.8 without regenerating their C files.

I'd be fine with PyCode_NewWithPosArgs (Victor's right that a descriptive naming convention handles future changes better than Ex)

[pablogsal]

I updated the PR and Victor reviewed it. Nick, could you review it as well?

[nascheme]

I suggest we change PyCode_New() back in the next beta, despite that change breaking Cython again. We could work with them so that they have a new release with a "PY_VERSION_HEX > 0x030800b1" test in it. Try to get that Cython version released before 3.8b2 hits. The window of a Cython version that doesn't support CPython >= 3.8b1 can be made pretty small. It would be nice to find these issues in alpha releases but finding them in beta is still early enough to fix it.

Having to regenerate all of the Cython emitted code embedded in different packages is painful. I experienced having to update numpy so that I could test my software with 3.8b1. Avoiding that is why we should change PyCode_New back. Introducing a new API like PyCode_NewWithPosArgs() is going to work better.

If we do have to make a similar incompatible change in the future, we should try hard to make it so there is an overlapping period of compatibility. It's bad to have an API change from one release to the next without giving time for projects like Cython time to catch up.

[vstinner]

If we do have to make a similar incompatible change in the future, (...)

IMHO the most important learnt lesson here is that we lack a proper Continuous Integration of the master branch of Python and popular PyPI projects. We should be notified *before* a release.

[scoder]

Note that PyCode_New() is not the only change in 3.8 beta1 that breaks Cython generated code. The renaming of "tp_print" to "tp_vectorcall" is equally disruptive, because Cython has (or had) a work-around for CPython (mis-)behaviour that reset the field explicitly to NULL after calling PyType_Ready(), which could set it arbitrarily without good reason.

So, either revert that field renaming, too, or ignore Cython generated modules for the reasoning about the change in this ticket.

I'm fine with keeping things as they are now in beta-1, but we could obviously adapt to whatever beta-2 wants to change again.

[pablogsal]

Adding Petr, that was involved with the "tp_print" to "tp_vectorcall" renaming.

[jdemeyer]

Technically, tp_print was replaced by tp_vectorcall_offset.

But that doesn't answer the question how we should deal with tp_print backwards compatibility. Cython does

FooType.tp_print = 0;

With this in mind, simply replacing tp_print by tp_vectorcall_offset is unsafe as it would break types that actually use vectorcall (there aren't many for now, but who knows how this will change in the future).

It would be safer to replace tp_print by tp_vectorcall since setting that to 0 won't break anything (neither for now, nor when PR 13930 is merged).

[jdemeyer]

PR 14009 deals with tp_print

[vstinner]

Note that PyCode_New() is not the only change in 3.8 beta1 that breaks Cython generated code. The renaming of "tp_print" to "tp_vectorcall" is equally disruptive, because Cython has (or had) a work-around for CPython (mis-)behaviour that reset the field explicitly to NULL after calling PyType_Ready(), which could set it arbitrarily without good reason.

Can someone please open a separated issue to discuss tp_print backward incompatible change? This issue is about PyCode_New().

[jdemeyer]

I think that the PyCode_New() compatibility problem and tp_print are sufficiently closely related that they can be discussed together.

In any case, I agree that it makes little sense to fix just one.

[vstinner]

Jeroen Demeyer:

I think that the PyCode_New() compatibility problem and tp_print are sufficiently closely related that they can be discussed together.

IMHO these problems are different enough to justify to have a separated issue: I created https://bugs.python.org/issue37250 Please discuss tp_print issue there.

[ncoghlan]

I'm happy with the change in #13959, but we need sign-off from Łukasz as release manager on bumping the Py_VERSION_SERIAL a bit early so Cython can reliably detect that this change has been reverted without having to check for the existence of the PyCode_NewWithPosArgs symbol.

Alternatively, if my proposal at cython/cython#3009 to check directly for PyCode_NewWithPosOnlyArgs in the Cython module setup code is accepted, then we wouldn't need to bump the version number early, and could merge the PR as soon as the What's New conflict is resolved.

[ncoghlan]

In reviewing cython/cython#3009, Jeroen pointed out that my symbol checking idea wouldn't actually work, since the preprocessor can only see preprocessor definitions, not compiler symbols. If we're going to rely on a preprocessor definition, it may as well be PY_VERSION_HEX, so I've updated the Cython PR accordingly.

That means we're back to either breaking Cython compatibility with CPython master until the release is cut, or else just bumping the release serial a little early. The duration of either state is now a lot shorter though, since the target release date for 3.8.0b2 is next week.

[vstinner]

That means we're back to either breaking Cython compatibility with CPython master until the release is cut

Yeah, that was my plan: merge PR 13959 "just before" beta2, and try to get a Cython release ASAP.

The best case would be to get a Cython release with the fix *before* beta2 is released. Would it be possible Stefan?

[scoder]

I'm really only waiting for bpo-37250 to be resolved, then I can prepare a new point release of Cython, preferably this week.

[encukou]

New changeset 4a2edc3 by Petr Viktorin (Pablo Galindo) in branch 'master':
bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper (GH-13959)
4a2edc3

[vstinner]

cython/cython#3009 was merged. Is it part of Cython 0.29.11 released yesterday?

[encukou]

cython/cython#3009 was merged.

(to master, so far)

Is it part of Cython 0.29.11 released yesterday?

It should be, see: https://cython.readthedocs.io/en/latest/src/changes.html#id2

[ambv]

New changeset cb083f7 by Łukasz Langa (Miss Islington (bot)) in branch '3.8':
bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper (GH-13959) (bpo-14505)
cb083f7

[scoder]

Is it part of Cython 0.29.11 released yesterday?

Yes.

[vstinner]

I reopen the issue. Cython 0.29.11 doesn't work on Python 3.8 beta2. Installation fails with a compiler error:

Cython-0.29.11/Cython/Plex/Scanners.c:7244:274: error: macro "__Pyx_PyCode_New" requires 16 arguments, but only 15 given

Cython commit 0d88839168013fd69350d31eaee5514cd2f727b9 is not part of Cython tag 0.29.11.

Stefan: it seems like a new Cython release is needed.

[vstinner]

I created cython/cython#3034 "Cython doesn't work on Python 3.8 beta2"

[scoder]

No need to keep this bug open on CPython side. The backwards compatibility has been restored (and I'll release Cython 0.29.12 today to resolve the issue on that side.)

[vstinner]

New changeset 1867b46 by Victor Stinner in branch 'master':
bpo-37221: PyCode_New() didn't change in Python 3.8 (GH-23595)
1867b46

[miss-islington]

New changeset d9d63f1 by Miss Islington (bot) in branch '3.8':
bpo-37221: PyCode_New() didn't change in Python 3.8 (GH-23595)
d9d63f1

[miss-islington]

New changeset ed46143 by Miss Islington (bot) in branch '3.9':
bpo-37221: PyCode_New() didn't change in Python 3.8 (GH-23595)
ed46143