classification
Title: PyCode_New API change breaks backwards compatibility policy
Type: compile error Stage: resolved
Components: Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: jdemeyer, lukasz.langa, nascheme, ncoghlan, pablogsal, petr.viktorin, scoder, serhiy.storchaka, vstinner
Priority: release blocker Keywords: 3.8regression, patch

Created on 2019-06-10 23:24 by ncoghlan, last changed 2019-07-07 09:10 by scoder. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 13959 merged pablogsal, 2019-06-11 01:48
PR 14505 merged miss-islington, 2019-07-01 10:36
Messages (32)
msg345153 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-06-10 23:24
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.
msg345154 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-10 23:28
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
msg345168 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-06-11 01:23
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 4f72a78684bbfcdc43ceeabb240ceee54706c4b0 (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.
msg345172 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-06-11 01:49
I have created PR13959 in case we decide to go with the PyCode_NewEx path.
msg345196 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-06-11 07:17
Seems it was changed last time many years ago and was stable in Python 3.
msg345229 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-06-11 12:08
> 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.
msg345230 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-06-11 12:27
> 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).
msg345271 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-11 21:44
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?
msg345272 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-11 21:47
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
msg345279 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-06-11 22:21
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)
msg345281 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-06-11 22:38
I updated the PR and Victor reviewed it. Nick, could you review it as well?
msg345283 - (view) Author: Neil Schemenauer (nascheme) * (Python committer) Date: 2019-06-11 22:58
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.
msg345284 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-11 23:08
> 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.
msg345305 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2019-06-12 04:42
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.
msg345311 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-06-12 08:47
Adding Petr, that was involved with the "tp_print" to "tp_vectorcall" renaming.
msg345312 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-06-12 09:38
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).
msg345313 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-06-12 09:49
PR 14009 deals with tp_print
msg345318 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-12 11:33
> 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().
msg345334 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-06-12 12:37
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.
msg345338 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-12 13:09
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.
msg346224 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-06-21 14:55
I'm happy with the change in https://github.com/python/cpython/pull/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 https://github.com/cython/cython/pull/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.
msg346389 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-06-24 12:46
In reviewing https://github.com/cython/cython/pull/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.
msg346405 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-24 14:06
> 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?
msg346427 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2019-06-24 18:20
I'm really only waiting for bpo-37250 to be resolved, then I can prepare a new point release of Cython, preferably this week.
msg346993 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2019-07-01 10:35
New changeset 4a2edc34a405150d0b23ecfdcb401e7cf59f4650 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)
https://github.com/python/cpython/commit/4a2edc34a405150d0b23ecfdcb401e7cf59f4650
msg346996 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-07-01 10:50
https://github.com/cython/cython/pull/3009 was merged. Is it part of Cython 0.29.11 released yesterday?
msg346997 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2019-07-01 10:54
> https://github.com/cython/cython/pull/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
msg347004 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2019-07-01 11:29
New changeset cb083f7cdf604c1d9d264f387f9e8846bc953eb3 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) (#14505)
https://github.com/python/cpython/commit/cb083f7cdf604c1d9d264f387f9e8846bc953eb3
msg347054 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2019-07-01 17:56
> Is it part of Cython 0.29.11 released yesterday?

Yes.
msg347414 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-07-06 02:11
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.
msg347415 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-07-06 02:14
I created https://github.com/cython/cython/issues/3034 "Cython doesn't work on Python 3.8 beta2"
msg347468 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2019-07-07 09:10
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.)
History
Date User Action Args
2019-07-07 09:10:14scodersetstatus: open -> closed
resolution: fixed
messages: + msg347468
2019-07-06 02:14:02vstinnersetmessages: + msg347415
2019-07-06 02:11:56vstinnersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg347414
2019-07-05 11:56:07petr.viktorinsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-07-01 17:56:16scodersetmessages: + msg347054
2019-07-01 11:29:18lukasz.langasetmessages: + msg347004
2019-07-01 10:54:48petr.viktorinsetmessages: + msg346997
2019-07-01 10:50:37vstinnersetmessages: + msg346996
2019-07-01 10:36:52miss-islingtonsetpull_requests: + pull_request14321
2019-07-01 10:35:10petr.viktorinsetmessages: + msg346993
2019-06-24 18:20:26scodersetmessages: + msg346427
2019-06-24 14:06:55vstinnersetmessages: + msg346405
2019-06-24 12:46:23ncoghlansetmessages: + msg346389
2019-06-21 14:55:00ncoghlansetmessages: + msg346224
2019-06-20 15:27:42jdemeyersetpull_requests: - pull_request13873
2019-06-12 13:09:45vstinnersetmessages: + msg345338
2019-06-12 12:37:54jdemeyersetmessages: + msg345334
2019-06-12 11:33:53vstinnersetmessages: + msg345318
2019-06-12 09:49:42jdemeyersetmessages: + msg345313
2019-06-12 09:48:30jdemeyersetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request13873
2019-06-12 09:38:07jdemeyersetmessages: + msg345312
2019-06-12 08:47:41pablogsalsetnosy: + petr.viktorin
messages: + msg345311
2019-06-12 04:42:27scodersetmessages: + msg345305
2019-06-11 23:08:35vstinnersetmessages: + msg345284
2019-06-11 22:58:42naschemesetnosy: + nascheme
messages: + msg345283
2019-06-11 22:38:30pablogsalsetmessages: + msg345281
2019-06-11 22:21:26ncoghlansetmessages: + msg345279
2019-06-11 21:47:35vstinnersetmessages: + msg345272
2019-06-11 21:44:00vstinnersetnosy: + vstinner
messages: + msg345271
2019-06-11 12:27:50pablogsalsetmessages: + msg345230
2019-06-11 12:08:01jdemeyersetnosy: + jdemeyer
messages: + msg345229
2019-06-11 07:17:40serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg345196
2019-06-11 06:51:24vstinnersetnosy: - vstinner
2019-06-11 01:57:15pablogsalsetnosy: + scoder
2019-06-11 01:49:09pablogsalsetkeywords: - patch

messages: + msg345172
stage: patch review -> needs patch
2019-06-11 01:48:19pablogsalsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request13825
2019-06-11 01:23:33pablogsalsetmessages: + msg345168
2019-06-10 23:28:10vstinnersetnosy: + vstinner
messages: + msg345154
2019-06-10 23:24:27ncoghlancreate