-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Introduce the Hybrid ABI #371
Conversation
…Instead of having cpython-ABI by default, we require it to be set explicitly. Also start to introduce supprt for the upcoming hybrid ABI
…distutils. Currently it's exactly the same as the universal ABI
… override hpy_abi
…at we can use hybrid on some files
…lusive again, and adjust the codebase
Before this commit, we always used .hpy.so, even on windows. Now do a much better job: 1. we use .hpy0.so instead of .hpy.so. We will migrate to .hpy1.so as soon as the ABI is stable 2. we use the proper extension on non-linux systems 3. for the hybrid ABI, we use a different scheme, which generates e.g. .hpy0-cp38.so on CPython (but with lots of different cases for all possible systems, see abitag.py and its test)
…in universal mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thank you!
I added some comments but I don't see blockers.
hpy/universal/src/hpymodule.c
Outdated
@@ -155,7 +155,7 @@ static PyObject *get_version(PyObject *self, PyObject *ignored) | |||
} | |||
|
|||
static PyMethodDef HPyMethods[] = { | |||
{"load", (PyCFunction)load, METH_VARARGS | METH_KEYWORDS, "Load a .hpy.so"}, | |||
{"load", (PyCFunction)load, METH_VARARGS | METH_KEYWORDS, "Load a .hpy0.so"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are also using this in hpy/devel/abitag.py
. I think these two usages should have a strong connection. Can we do what we do with the version? Define it in a C header and expose it in Python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One little more thing: there is an int ctx_version
field in HPyContext
. Matti brought to my attention, that this field basically is the ABI version, right? So, I think we should set ctx_version
appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, good point. It turns out that sharing the same definition in C and Python is a mess as usual, so I opted for defining it in two places (hpy.h and abitag.py) and add a test which checks that they are on sync.
Also, I renamed ctx->ctx_version
into ctx->abi_version
and set its value to HPY_ABI_TAG
, see commits 71d2a7d and 1f70e65.
test/test_legacy_forbidden.py
Outdated
# We need this because some of the nice compilation errors (such as the ones | ||
# causes by _HPY_LEGACY) are triggered only by gcc. Would be nice to have them | ||
# also for other compilers | ||
ONLY_GCC = pytest.mark.skipif(sys.platform!='linux', reason='GCC only') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that maybe go into support.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed by 4907ee8
If we include the ABI version in the tag, I am wondering if we need #356. At the same time I like the approach in #356, because it will be "fool-proof" -- even if you have wrong tag for whatever reason it'll still check and choose the right ABI for your extension. So right now I am in favor of keeping both the ABI tag and #356 even though they are redundant. |
@steve-s You are right, I forgot about that effort. Would it make sense to introduce some generic ABI tag? |
What exactly do you mean by that? That we'd drop |
Yes, that's what I meant. I do also think that a generic ABI tag does not make a lot of sense. I agree that we can have both. |
Is there more to do here or should we merge this as-is and incrementally improve things toward a release? |
Closes #366 |
I also had a cursory look over the changes and I vote for merging it and incrementally improve things toward a release |
@antocuni would you like to make one last pass over the comments and clear the merge conflict? |
…e the hpy_abi fixture
Co-authored-by: Florian Angerer <[email protected]>
after the capsule changes, this PR has conflicts |
if think we can have both: basically, the logic of this PR tries to ensure that you cannot use CPython symbols by mistake, and #356 would become a sanity check to ensure that it works well. |
…antocuni/hybrid-abi
… and fix conflicts about capsules
LGTM. Please go ahead and merge. |
Yay! |
This PR introduces the
hybrid
ABI:universal
cannot#include <Python.h>
, cannot use legacy features and produce a file called e.g.simple.hpy0.so
.hybrid
can usePython.h
and produce a file called e.g.simply.hpy0-cpy38.so
.See the updated docs for more info.
Filename
For universal, I choose to use
.hpy0.so
because we should use a versioned scheme. I didn't want to use.hpy1.so
for now, because we might still want to change the ABI before becoming "official"For hybrid, we want to include both the hpy version and the cpython version. I chose
hpy0-cp38.so
, which is the result of many sub-choices.To start with, let's see what is the filename that you get on "normal" extensions:
foo.cpython-38-x86_64-linux-gnu.so
.foo.abi3.so
foo.cpython-310-darwin.so
(or.dylib
? I think I've seen both)foo.cp38-win_amd64.pyd
foo.pypy38-pp73-x86_64-linux-gnu.so
foo.graalpy-38-native-x86_64-darwin.dylib
It varies wildly:
cpython-38
andcp38
.So for hpy hybrid I chose to:
cp38
instead ofcpython-38
: this is shorter, and most importantly it's the same tag which goes in the wheel namesCompile time errors
In universal mode, you are not allowed to use
Python.h
: we try hard to display a nice compile time error by putting our ownPython.h
in the include dirs. This might not work in all setups, but it should be good enough for most cases.In universal mode, you cannot call
HPy_AsPyObject
andHPy_FromPyObject
: I managed to get a compile-time error on gcc, but not on old clangs nor in MSVC. Again, too bad but it's better than nothing:hpy/hpy/devel/include/hpy/cpy_types.h
Lines 24 to 27 in 4cde717
hpy/hpy/devel/include/hpy.h
Lines 76 to 95 in 4cde717
The other legacy features that you cannot use are
.legacy_methods
and.legacy_slots
: but these are now impossible to use, because in order to pass anything meaningful you have to instantiate e.g. aPyMethodDef
struct, which you can't because you cannot includePython.h
We still need to reference some
cpy_*
types, e.g. to declare theHPyModule_Spec.legacy_methods
field. This is done here:hpy/hpy/devel/include/hpy/cpy_types.h
Lines 4 to 36 in 4cde717
in cpython and hybrid modes, we use the real types from
Python.h
. In universal mode, we use "fake" types, but note that they are only forward declarations and they cannot be instantiated.This should also fix #288.
Future improvements
There are many things which we can to to improve the situation, but I think they can be done in future PRs:
universal
, we could run a sanity check to actually ensure that we don't link against anyPy*
symbolHPy_{As,From}PyObject
or use.legacy_methods
, etc.