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

PEP 757: PyLong_Export() can fail #4025

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 8, 2024


📚 Documentation preview 📚: https://pep-previews--4025.org.readthedocs.build/

@vstinner
Copy link
Member Author

vstinner commented Oct 8, 2024

cc @encukou @skirpichev

@skirpichev
Copy link
Member

skirpichev commented Oct 8, 2024

Obviously, I would prefer to keep this constraints, unless other workarounds for @encukou concerns are impossible. Removing it makes API less convenient.

@encukou
Copy link
Member

encukou commented Oct 8, 2024

Workarounds are possible, but IMO worse. I can think of:

  • put a version in PyLongLayout. Then we can expand the PyLongLayout struct in the future, or even (as a big stretch) mark the whole concept as non-functional.
  • add this as Unstable API, so it can be removed without deprecation.

@skirpichev
Copy link
Member

put a version in PyLongLayout.

I would reiterate, that PyLongLayout has nothing to say about small integers. I hope #4026 will clarify this. We can add a version field to this structure as well, but why? To allow bigint representations, different from best libraries in the field?

But we could put (you original proposal) version to the PyLongExport structure.

add this as Unstable API, so it can be removed without deprecation.

The version field looks much better. Users just have to put upper bound for CPython version. If new one is coming with a new variant of PyLongExport - they just will have to add a new if (layout->version == 123) { ... }...

But suspect, that it's overkill and int64_t value will fit our needs for 20+ years.

@vstinner
Copy link
Member Author

vstinner commented Oct 9, 2024

The alternative is to keep the "cannot fail" sentence and implementation a deprecation by using Py_DEPRECATED() macro on the function, and remove the function after a deprecation cycle (ex: 2 Python releases). It has been done in the past. It's not perfect, but better than nothing.

@skirpichev
Copy link
Member

Assuming this removal, how this helps for possible deprecation of this API?

The alternative is to keep the "cannot fail"

In fact, the whole API is about big integers. @encukou, do you think we can do API for reading big integers with such constraint? If we can't offer zero-copy mechanism (i.e. mpz_limbs_read-like for reading) - such API will be useless.

@vstinner
Copy link
Member Author

vstinner commented Oct 9, 2024

Assuming this removal, how this helps for possible deprecation of this API?

When a function is modified to emit a DeprecationWarning, the warning can be treated as error in which case, the function starts failing.

@encukou
Copy link
Member

encukou commented Oct 10, 2024

If we can't offer zero-copy mechanism (i.e. mpz_limbs_read-like for reading) - such API will be useless.

Indeed. If the internal representation changes, this API will be useless.

However, I realized the user must already branch to two cases (compact and big integers). How onerous would it be to add two more (failure with exception, and an “unknown/future format” case that can keep the API from becoming entirely useless if the representation changes)? Please see the discussion thread: https://discuss.python.org/t/63895/51

@vstinner
Copy link
Member Author

How onerous would it be to add two more (failure with exception, and an “unknown/future format” case that can keep the API from becoming entirely useless if the representation changes)?

Currently, in the gmpy2 project, there are 27 calls to the mpz_set_PyLong() function which convert a Python int to a GMP mpz. mpz_set_PyLong() would call PyLong_Export() with PEP 757. Currently, mpz_set_PyLong() cannot fail.

If PyLong_Export() can fail, the 27 calls should be adapted to handle error which is not convenient. It's the same in other projects which currently rely on Python internals (access directly PyLongObject members).

@encukou
Copy link
Member

encukou commented Oct 11, 2024

gmpy2 may choose to modify mpz_set_PyLong so that on failure it aborts the process, or returns zero. That way, the expected behaviour, on new CPython versions that change the internal representation, would remain the same as with the internal API.

I guess we can guarantee the export won't fail in 3.14. But the point here is that it's not version-specific API any more.

@skirpichev
Copy link
Member

That does make sense if PyLong_Export() can fail only if internal representation was changed with the new release. So, eventually, API will just stop to work abruptly.

I like more your approach in the d.p.o thread, where API doesn't fail on integers and we just return some export code (might be new in the new release).

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.

3 participants