Support stable Python ABI #1613
Conversation
|
Were you expecting any tests to pass? Looks like every test fails with |
I am not expecting this PR to be merged as-is, I created for people on the thread to comment on proposed changes. So far there were no takers... Maybe you'd care to take a look? :) The error is injected on purpose, so I could be sure I was really testing with the |
|
Hi all, we (www.hex-rays.com) are long-time SWiG users, and we will soon have to ship our product with support for Python 3. I can see that the work on this branch is not finished, but at a first glance it seems like a good start. |
vadimcn
force-pushed
the
py3-stable-abi
branch
2 times, most recently
from
e0be864
to
648ac95
Compare
|
Sorry, I still didn't have time to test this in my real projects using SWIG, which I'd like to do. But just looking at the changes they do seem fine, thanks. |
|
I think there is still an issue, on windows.
and
but -- Edit: ah, I just spotted this by @ojwb : 4aaf48d
which goes directly against the idea of SWiG using its own mutexes. |
|
Are you saying Python no longer provides a public thread C API at all? Or is this only with The SF patch referenced is now at: https://sourceforge.net/p/swig/patches/175/ It was long ago, but I think the key motivation was probably that it was broken on a platform I don't use (but people using the code I generate with SWIG want to use) and it was simpler to fix it with a cross-platform solution. However it seemed (and still seems) cleaner to use Python's threading abstraction layer rather than heading straight for a platform-specific threading layer. I'd imagine all POSIX platforms have pthreads by now, but Microsoft of course have their own different APIs for almost everything. And if there's more than one thread implementation available on a platform I'd rather not have to try to think about what happens if Python and the module use different ones. I don't know if any current platforms do, but e.g. Linux used to have both LinuxThreads and NPTL. |
|
If a function isn't in the limited API, build should have failed at the compilation stage. |
|
I wonder if it needs to be a separate lock, or can we simply reuse the GIL? |
|
If attempting to use the GIL, beware of the simplified GIL State API (which I think SWIG still uses by default) as this can cause problems (or at least used to - it's possible things have evolved since) - e.g. see https://code.google.com/archive/p/modwsgi/wikis/ApplicationIssues.wiki and search for the "Python Simplified GIL State API" heading. For Xapian we actually replace SWIG's code with our own version which uses the Python thread API but not the GIL State API (and as a bonus does nothing if |
|
@ojwb: the article you referenced indeed suggests that extensions using simple gil interface may deadlock if used in environment with multiple sub interpreters. However it does not explain the exact mechanism of this happening, so I was unable to tell whether this condition would apply to our case. |
|
@ojwb I should have been more clear, sorry. The function is indeed not excluded from the Py_LIMITED_API, but linking will fail due to it not being exposed in python3.dll @vadimcn I guess it can be considered a CPython bug. I've found this relevant discussion: https://mail.python.org/pipermail/python-dev/2015-April/139124.html But, the way I see it, whether or not it is a CPython bug (and ideally will be fixed in the future) the whole idea of using Py_LIMITED_API is that we can ship SWiG-generated wrappers that will run against existing Python 3 installations (provided they are not ancient) as well as future installations. Therefore, I don't think it's a great course of action to expect CPython developers to fix it, and then use it, since that means SWiG would only be able to support Python 3.9 & later. Edit: just for the record (I know that's probably not an acceptable solution) I just put the old code back in place, and it works in our case. |
This used to be broken in wxQt until it was fixed by the previous commit. See swig/swig#1613
It affects SWIG-generated extensions by default, but it's possible (and fairly easy) to override that and get an extension which doesn't use this problematic simplified GIL locking API.
Using the GIL is not a problem in itself. The problem is using the simplified GIL locking API (
I'd really recommend against adding further uses of the simplified GIL API - I think it's a mistake SWIG is using it at all - but in practical terms what I really care about is that it doesn't become harder to stop SWIG using it in the bindings I maintain. Currently in our interface file we just need to define |
|
I don't think anything is blocking this PR. It may be incompatible with |
|
If it's going to be incompatible with Generating code which fails to compile or fails to work is just going to generating support load, and we already struggle to keep up with tickets. |
|
You should probably also remove the |
vadimcn
force-pushed
the
py3-stable-abi
branch
from
0696617
to
bd9f159
Compare
vadimcn
force-pushed
the
py3-stable-abi
branch
2 times, most recently
from
a906626
to
fd43207
Compare
|
Any ideas how to make AppVeyor produce a more useful error message? |
No, but it also seems to be failing on master currently - see this build from 11 days ago: https://ci.appveyor.com/project/swig/swig/builds/28735588 The exact same commit apparently built OK 18 days ago: https://ci.appveyor.com/project/swig/swig/builds/28555074 So some change in the appveyor environment I guess. |
Yes, I've already commented on this. I.e. basically |
|
Well, other than that, this PR is ready to be merged, I think. |
|
hi, @vadimcn, could you rebase on top of master ? Files are conflicting. |
|
@jschueller: Looks like only the documentation html is in conflict. This should not prevent you from trying out this change. |
|
@vadz: are you still working to land this PR? |
|
Sorry, but unfortunately I can't really do much. I could rebase it and solve the minor issue mentioned in #1613 (comment) but normally only @wsfulton merges PRs and he hasn't shown any sign of life since quite some time (it would be reassuring to hear something just to know that you're ok, William!). FWIW I'm using this in a couple of my projects and am very happy with it, thanks again for your changes! |
| { Py_tp_doc, (void*)varlink__doc__ }, | ||
| { 0, NULL } | ||
| }; | ||
| PyType_Spec spec = {}; |
There was a problem hiding this comment.
These empty initializer lists pose a problem for some compilers, notably more recent versions of MSVC, or when using strict compile options (e.g. gcc with -Wpedantic).
There was a problem hiding this comment.
This is indeed not ISO C and we should avoid it:
$ echo 'struct { int x; } a = {};'|gcc -pedantic -x c - -o /dev/null
<stdin>:1:23: warning: ISO C forbids empty initializer braces [-Wpedantic]
The reason for this is presumably to 0-initialise members not explicitly initialised below. The Python docs for this structure explicitly enumerate the members, and the only one not explicitly initialised here is itemsize, so we should be able to drop the {} and add spec.itemsize = 0; below.
There seem to be two more instances of this.
|
I am also using the code in this PR (#1613 ) and #1687 on Windows and Linux with Python 3.6-3.8 and I did not encounter any issues for a couple of month now. So thanks to @vadimcn and @vadz for this code! @wsfulton is there any chance this will be consider to go into the mainline and can I maybe help to support this two PR? |
|
I've added this to the swig-4.1 milestone (consideration for next release). |
|
Friendly reminder, is there any update on this? Would be great to have it in! |
|
@vadz Apparently the missing |
|
Yes, thanks, I've seen that we can have a proper solution with 3.10, but we've been using this with anything >= 3.5 for, what, a couple of years already by now, so I don't think it's such a problem to support older versions. The real problem is that there is still no SWIG release integrating this, but I don't know what could I do to help with this (other than maintaining my own fork of SWIG which I'm not quite ready to do yet...). |
|
The stable Python ABI support would also for older (>=3.5) Python versions be really nice. |
|
@wsfulton If I rebase this on latest master and add the CI builds using |
|
Push, I'm still looking forward to get the stable abi working with swig. If I could implement or test something, let me know. |
|
FWIW this is broken again with the latest master as 84ff84f added uses of This is not really difficult to fix, and I did it in my own branch, but as I've never got any reply to my previous comment I don't know if I should bother doing anything here. I really don't know what else can I say or do, this is an incredibly important PR, I'd say that it's completely indispensable for any use of SWIG with Python (and definitely with closed source libraries), yet it languishes here since more than 2 years. |
|
While I think you're overstating the importance of this (I use SWIG with Python and have not found a need for this) but it does seem like it ought to be merged, and looks plausible. I'm not comfortable with doing that myself though. |
Yes, this is what I did. This is not totally trivial, however, because the function and the macro have different semantics (which seems like an awful idea to me but what do I know about API design), with the function decref'ing the previous item, unlike the macro. It seems that it doesn't matter for its use in the SWIG code, so we can do it like this, but it's not very nice to depend on this continuing to be the case in the future and we may end up silently starting to crash in limited API builds if master adds another use of this macro in some place where the difference matters. Which means I know need to track this somehow and remember to check for this when I merge with master the next time, which is something that I really could do without...
Do you really ship binaries for all of Python 3.4, 3.5, 3.6, 3.7, 3.8, 3.9 and 3.10?? We only did this for 3.[4-6] which were the only versions used before we started using this PR and it was already bad enough, I can't imagine doing it now.
I understand this perfectly, but I'm just frustrated because I don't understand what's missing here, it looks like this could, and should, have been merged a very long time but nothing happens, even though other, vastly less important PRs, do get merged from time to time. Anyhow, it's probably too late to do it this year. Hopefully it will get merged in 2022... |
No, I don't ship binaries. As to what binary-shipping Linux distros do, I know that Debian and derivates handle this by building for each supported minor version (which is pretty much all automated by the packaging tools). There's typically only a few supported at a time - e.g. Debian unstable currently builds for 3.9 and 3.10 (the quick way to see is to check which python3*-dev packages python3-all-dev depends on). This stable ABI would probably be helpful for someone creating a new binary Linux distro from scratch now, but I'd imagine all the existing ones built machinery to handle this long ago. |
|
Right, for open source projects it's not much of a concern -- unless they want to support Windows, too, where people are much less ready to build their own extensions. But if you have clients using all the various Python 3 versions under Windows, limited API is a life saver. |
|
Even using cibuildwheel to build all your binaries (3.6-3.10, no one is using 3.4 or really even 3.5, those are well past EOL), having 1 binary instead of 5 reduces your PyPI upload requirements by 5x, and keeps you from having to upload new wheels on every Python release in October. So it's a pretty big deal. Though it's pretty lightly used in practice (even still has some almost-bugs, like not supporting platform tags, so you can't put different OSs and architectures in the same folder, like for a zipapp), largely because most systems don't really support it (this PR for SWIG, I think Cython 3 alphas have some support, pybind11 can't at the moment, etc) |
|
I have been using swig with this PR for a while. It is a real life-saver when you have a large binary python wheel. Uploading such a wheel for every python version to pypi would quickly use up the project size limits. Thank you so much for this work @vadimcn and @vadz! I hope this will eventually get merged into swig, so far I have been using a fork with this PR, together with #1687 which is also very useful. |
On Windows, we currently ship 2.7, 3.5, 3.6, 3.7, 3.8, 3.9 and there is a request for 3.10. All of them for x86 and for x64. We've patched the generated .py file to look for a path in the Windows registry, set the sys.path corresponding to the python version, and continue with the pyd. I'm talking about 3 MB of generated cxx file and a 3.7 MB pyd for each of them (2.9 MB for x86). All of this to connect a commercial DLL of 5.5 MB. Oh, is there a point in talking about .pdb or the compilation time of a 3 MB cxx file? So I'd like to answer: Yes! |
|
I'm not really sure how much sense does it make to continue this discussion, but the question is not whether it's possible to ship all these binaries -- the answer is definitely "yes" and, as I wrote, I used to do it myself. But if the question is whether I want to go back to it instead of shipping just a single binary for all Python 3 versions, then the answer is even more definitely "no". |
|
Sure, I'm on your side here. I hope my point was clear. It is insane being forced to ship every single 3.x pyd. I'm all for the stable API. |
|
This probably should be closed in favour of #2190 too now. |
No description provided.