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

[ENH] Use pythoncapi_compat.h in Cython? #3934

Closed
vstinner opened this issue Dec 4, 2020 · 4 comments
Closed

[ENH] Use pythoncapi_compat.h in Cython? #3934

vstinner opened this issue Dec 4, 2020 · 4 comments

Comments

@vstinner
Copy link
Contributor

vstinner commented Dec 4, 2020

Hi,

I created pythoncapi_compat.h header file to ease support multiple Python versions. In practice, it provides new functions of the Python C API to old Python versions (up to Python 2.7, including Python 3.5).

https://github.com/pythoncapi/pythoncapi_compat

Would it make sense to use it in Cython?

Cython/Utility/ModuleSetupCode.c contains code like:

#define __Pyx_NewRef(obj) (Py_INCREF(obj), obj)

#if PY_VERSION_HEX >= 0x030900A4
  #define __Pyx_SET_REFCNT(obj, refcnt) Py_SET_REFCNT(obj, refcnt)
  #define __Pyx_SET_SIZE(obj, size) Py_SET_SIZE(obj, size)
#else
  #define __Pyx_SET_REFCNT(obj, refcnt) Py_REFCNT(obj) = (refcnt)
  #define __Pyx_SET_SIZE(obj, size) Py_SIZE(obj) = (size)
#endif

pythoncapi_compat.h provides Py_SET_REFCNT(), Py_SET_SIZE() and Py_NewRef() on all Python versions (use existing functions if available). The advantage is use regular Py_xxx() functions rather than having to use custom __Pyx_xxx() functions.

The problem is that I don't understand Cython is implemented, and so I'm not sure how to include an header file in ModuleSetupCode.c for example.

Victor

@da-woods
Copy link
Contributor

da-woods commented Dec 5, 2020

I definitely don't make the decision on this but I guess the issues would be:

  • Currently Cython modules depend only on the Python headers and the C standard library (which is about as minimal dependencies as possible). This adds a new dependency, so if Cython did use it then would probably need to distribute pythoncapi_compat.h too (rather than relying on users installing it).
  • ModuleSetupCode.py includes compatibility workarounds for other implementations (mainly PyPy, but it looks like Pyston too). It doesn't immediately look like these workarounds overlap with what you've got in pythoncapi_compat.h but it's potentially something that Cython needs to support than may not be in your scope.

@vstinner
Copy link
Contributor Author

vstinner commented Dec 5, 2020

This adds a new dependency

Technically, I'm not sure that producing a #include "pythoncapi_compat.h" in the final C code would be convenient. A more convenient approach would be put copy/paste the pythoncapi_compat.h content in the C code directly (as the C preprocessor already does for includes), to not have to install the file on the system (outside Cython).

Also, I suggest to keep a copy of pythoncapi_compat.h in your Cython, and only update it when it's needed.

The idea is to externalize the maintenance of these "compatibility" functions outside Cython, to reduce Cython maintenance burden.

ModuleSetupCode.py includes compatibility workarounds for other implementations (mainly PyPy, but it looks like Pyston too).

pythoncapi_compat.h is part of a larger project to make the C API more abstract to better support Python implementations other than CPython. I'm interested to support PyPy, Pyston, etc. Right now, pythoncapi_compat.h only support a few functions and I didn't test it on PyPy or Pyston yet. I expect that it just works since it relies on functions which exists for a long time like Py_TYPE().

Maybe using pythoncapi_compat.h in Cython right now doesn't provide any value and only gives you more work. If it would the case, it would be a bad idea ;-)

@robertwb
Copy link
Contributor

robertwb commented Dec 6, 2020

Some of the code in Cython certainly has a lot of overlap with what's being done in pythoncapi_compat.h, and it may make sense to join forces in the long run, but I'm not sure it has much value right now.

In particular, both taking on an additional dependency and shipping it as part of Cython itself have downsides (especially when it comes to versioning, and in cases that user wanted to use a library that itself included pythoncapi_compat.h). We also try hard to avoid defining symbols arbitrarily in the global namespace (by near ubiquitous use of the __pyx prefix) and there is value in consistency as well (if we used pythoncapi_compat.h, we could only do so for the subset that is already defined, and presumably only that functionality that is part of the latest Python's public API).

It seems to me that the primary value of pythoncapi_compat.h is amortizing the cost of supporting multiple Python versions (and, possibly, eventually a more limited C API) across a large number of modules written against the Python C API. In some sense though, Cython is already achieving this amortization for a huge number of extension libraries, so the cost/value of doing this by hand is very different than for a one-off hand-coded C module.

In summary, I would say it's worth keeping an eye on, but simply using pythoncapi_compat.h as is would probably not be worth it at this point.

@vstinner
Copy link
Contributor Author

vstinner commented Dec 7, 2020

We also try hard to avoid defining symbols arbitrarily in the global namespace (by near ubiquitous use of the __pyx prefix) and there is value in consistency as well (...)

I see, that makes sense.

I close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants