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

Prevent abi3 from being used with no-GIL interpreter #4424

Merged
merged 5 commits into from
Aug 14, 2024

Conversation

abravalheri
Copy link
Contributor

Summary of changes

Closes #4420

Pull Request Checklist

@abravalheri abravalheri requested a review from dholth June 17, 2024 18:29
@abravalheri
Copy link
Contributor Author

@dholth, @bastimeyer, this is a draft that expands on #4420 (comment) (I just wanted to make sure the information why abi3 is not used is present in the logs).

Does it make sense to still use the given py_limited_api and just ignore abi3, or we should also discard the given py_limited_api? (If the second option is preferable, then it would make more sense to me to raise an exception due to incompatible configuration).

I don't have an approach to test this in the CI yet... Suggestions are welcome.

@@ -363,6 +354,24 @@ def get_tag(self) -> tuple[str, str, str]:
), f"would build wheel with unsupported tag {tag}"
return tag

def _get_tag_impure(self, plat_name: str) -> tuple[str, str, str]:
impl_name = tags.interpreter_name()
impl_ver = tags.interpreter_version()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained in the issue thread, impl_ver does not include abi flags. Here's the implementation of tags.interpreter_version():

def interpreter_version(*, warn: bool = False) -> str:
"""
Returns the version of the running interpreter.
"""
version = _get_config_var("py_version_nodot", warn=warn)
if version:
version = str(version)
else:
version = _version_nodot(sys.version_info[:2])
return version

>>> import sys,sysconfig
>>> sysconfig.get_config_var("py_version_nodot")
'313'
>>> sys.abiflags
't'
>>> sys.version_info[:2]
(3, 13)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bastimeyer.

It seems that we can use sys.abiflags then...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the code to reflect my latest understanding.

@abravalheri abravalheri force-pushed the issue-4420 branch 2 times, most recently from ad24375 to 40d29d2 Compare June 19, 2024 08:41
f"`Py_GIL_DISABLED` ({sys.abiflags=!r}).",
see_url="https://github.com/python/cpython/issues/111506",
)
self.py_limited_api = False
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to deal with the following problem however:

The extensions themselves define py_limited_api like in https://github.com/Legrandin/pycryptodome/blob/39b5a627dab9124da0fb9d15a2f3a19434d96565/setup.py#L286C9-L286C28.

Not sure how that will interact with this.

Maybe the best is to just raise an exception and require the developer to be mindful about the compatibility with py_limited_api (and change their setup.py accordingly to react to the current version of Python being used)?

Copy link
Member

@dholth dholth Jun 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that really break, or is a "py_limited_api" extension, compiled for the nogil intepreter, just not cross-compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit out of my comfort zone for the time being, but my understanding of the discussion on python/cpython#111506, https://discuss.python.org/t/python-abis-and-pep-703/34018 is that there is potential for break things.

The following hints in https://peps.python.org/pep-0703/#backwards-compatibility seem to suggest to me that some parts of the limited API may currently require (or at least assume) the GIL to work properly (therefore extensions using those parts of the limited API are potentially prone to runtime errors).

  • C-API extensions that rely on the GIL to protect global state or object state in C code will need additional explicit locking to remain thread-safe when run without the GIL.
  • C-API extensions that use borrowed references in ways that are not safe without the GIL will need to use the equivalent new APIs that return non-borrowed references. Note that only some uses of borrowed references are a concern; only references to objects that might be freed by other threads pose an issue.
  • Custom memory allocators (PyMem_SetAllocator) are required to delegate the actual allocation to the previously set allocator. For example, the Python debug allocator and tracing allocators will continue to work because they delegate the allocation to the underlying allocator. On the other hand, wholesale replacing of the allocator (e.g., with jemalloc or tcmalloc) will not work correctly.
  • Python objects must be allocated through the standard APIs, such as PyType_GenericNew or PyObject_Malloc. Non-Python objects must not be allocated through those APIs. For example, it is currently acceptable to allocate buffers(non-Python objects) through PyObject_Malloc; that will no longer be allowed and buffers should instead be allocated through PyMem_Malloc, PyMem_RawMalloc, or malloc.

@abravalheri abravalheri force-pushed the issue-4420 branch 3 times, most recently from 2fbb280 to aaff540 Compare June 25, 2024 10:08
@bastimeyer
Copy link

abravalheri requested a review from bastimeyer last week

#4424 (comment)

I don't know how to deal with the following problem however:

The extensions themselves define py_limited_api like in Legrandin/pycryptodome@39b5a62/setup.py#L286C9-L286C28

I'm sorry, but I can't help with this, because as explained in the issue thread, I don't have any experience with Python C-extensions, so I don't know the specific requirements and the best way forward in order to solve this. The issue was opened by me purely from the perspective of someone who has a dependency on a project with C-extensions and abi3 wheels, where I ran into these build issues.

The maintainers of those projects which build extensions based on the limited API will need to provide feedback. I've already asked the maintainers of pycryptodome in the issue I've opened there, but I can ask again and link to this pull request, too.

@rgommers
Copy link

rgommers commented Jul 10, 2024

Automatically disabling building for the limited API or raising a clear exception telling the user to set a flag to explicitly disable use of the limited API are both reasonable options I believe. As a data point: meson-python does the latter (see mesonbuild/meson-python#621 and docs) while scikit-build-core does the former (see scikit-build/scikit-build-core#741 and docs).

@abravalheri abravalheri marked this pull request as ready for review August 12, 2024 15:51
@abravalheri
Copy link
Contributor Author

Based on the feedback in #4424 (comment), I opted to raise a clean exception instead of the warning.

The message the user will see is something like:

`py_limited_api='cp35'` not supported. `Py_LIMITED_API` is currently incompatible with
`Py_GIL_DISABLED` (sys.abiflags='t'). See https://github.com/python/cpython/issues/111506.

which make it easier to identify the root cause of the problem and is better than what they currently see:

AssertionError: would build wheel with unsupported tag ('cp35', 'abi3', 'linux_x86_64')

ERROR Backend subprocess exited when trying to invoke build_wheel

Comment on lines +299 to +304
raise ValueError(
f"`py_limited_api={self.py_limited_api!r}` not supported. "
"`Py_LIMITED_API` is currently incompatible with "
f"`Py_GIL_DISABLED` ({sys.abiflags=!r}). "
"See https://github.com/python/cpython/issues/111506."
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaraco do you agree with this approach?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. I don't have more than a superficial understanding of the concern. For example, it's unclear to me if this limitation is temporary and if so, under what conditions could the restriction be lifted. I hope in the future some people will bring more insight and help provide more robust support for the various combinations (if feasible).

@abravalheri abravalheri merged commit 57379cd into pypa:main Aug 14, 2024
19 of 21 checks passed
@abravalheri abravalheri deleted the issue-4420 branch August 14, 2024 09:18
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

Successfully merging this pull request may close these issues.

[BUG] Can't build wheels on CPython 3.13t (no GIL / PEP 703) when bdist_wheel's py-limited-api option is set
5 participants