GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-106320: Re-add some PyLong/PyDict C-API functions by scoder · Pull Request #111162 · python/cpython

Conversation

@scoder

@scoder scoder commented Oct 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Re-add _PyLong_FromByteArray(), _PyLong_AsByteArray(),_PyLong_GCD() and _PyDict_Pop() to the public header files.
They are used by third-party packages and there is no efficient replacement.

See #111140
See #111139
See #111262

  • Issue: C API: Remove private C API functions (move them to the internal C API) #106320

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jcrist, encukou, and osandov reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@scoder
pythongh-106320: Re-add _PyLong_FromByteArray(), _PyLong_AsByteArray(…
8d46cb3
…) and _PyLong_GCD() to the public header files since they are used by third-party packages and there is no efficient replacement.

See python#111140
See python#111139
@scoder
scoder requested review from corona10 and rhettinger as code owners October 21, 2023 12:29
@bedevere-app bedevere-app Bot added the awaiting core review label Oct 21, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Oct 21, 2023
C API: Remove private C API functions (move them to the internal C API) #106320
Closed
This was referenced Oct 21, 2023
C API: Consider adding a public PyLong_GCD() function #111139
Closed
C API: Consider adding public PyLong_AsByteArray() and PyLong_FromByteArray() functions #111140
Closed
@sobolevn
sobolevn requested a review from vstinner October 21, 2023 12:30
@scoder scoder added the skip news label Oct 21, 2023
@scoder

scoder commented Oct 21, 2023

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka @vstinner

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Oct 21, 2023

Copy link
Copy Markdown
Member

I would prefer to take it as an opportunity to add a nice public API for that. In the meanwhile (until next alpha release), you can just use the internal C API.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@scoder

scoder commented Oct 22, 2023 via email

Copy link
Copy Markdown
Contributor Author
add a nice public API for that
Oh, I'd be happy to. I see the revert as a way to make sure there *is* an API until we have something to replace it. For which, as Serhiy noted, the design and discussion can take arbitrarily long.
encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@scoder

scoder commented Oct 22, 2023

Copy link
Copy Markdown
Contributor Author

Just for the record, I micro-benchmarked _PyLong_AsByteArray() against the C-API call to int.to_bytes() and the incremental unpacking of 62 bit chunks using PyNumber_And(), PyLong_AsLong() and PyNumber_Rshift(). The code complexity increases substantially from first to last in that order.

# int.to_bytes()
$ python3.10 -m timeit -s 'from intbench import i128_from_py' 'i128_from_py(2**126)'
200000 loops, best of 5: 1.06 usec per loop

# incremental unpacking
$ python3.10 -m timeit -s 'from intbench import i128_from_py' 'i128_from_py(2**126)'
500000 loops, best of 5: 693 nsec per loop

# _PyLong_AsByteArray()
$ python3.10 -m timeit -s 'from intbench import i128_from_py' 'i128_from_py(2**126)'
500000 loops, best of 5: 504 nsec per loop

This suggests to me that making _PyLong_AsByteArray() public is worth it. The replacements are just horrible to implement, and the "obvious replacement" is really slow.

jcrist reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Oct 23, 2023

Copy link
Copy Markdown
Member

I would prefer adding these back, so that we can take our time to add a proper replacement.
Or, Victor, do you think you can design and add a good replacement in time for the next alpha?

mdickinson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@osandov osandov mentioned this pull request Oct 23, 2023
Handle private APIs removed in Python 3.13 osandov/drgn#361
Closed
@scoder scoder mentioned this pull request Oct 25, 2023
Make _PyDict_Pop() public #111262
Closed
@scoder
pythongh-111262: Re-add _PyDict_Pop() to have a C-API until a new pub…
794eb8a
…lic one is designed.
@scoder
scoder requested a review from kumaraditya303 as a code owner October 25, 2023 08:08
@scoder scoder changed the title gh-106320: Re-add some PyLong C-API functions gh-106320: Re-add some PyLong/PyDict C-API functions Oct 25, 2023
encukou
encukou approved these changes Oct 25, 2023
View reviewed changes

@encukou encukou left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

I still don't see the reason why this API was removed. PEP-387 says that it can be removed, not that it should.
We don't have a good replacement for this API. After we add that, it'll be time to start telling people to change their code.

I take @gvanrossum's comment on another issue as support for reverting the removal:

I strongly prefer revert over fix, for removed APIs that cause problems.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Oct 25, 2023
@encukou
encukou merged commit a8a89fc into python:main Oct 25, 2023
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 25, 2023
@scoder
scoder deleted the readd_pylong_capi branch October 25, 2023 11:12
@vstinner

vstinner commented Oct 27, 2023

Copy link
Copy Markdown
Member

Victor, do you think you can design and add a good replacement in time for the next alpha?

Yes, I plan to add public APIs to replace these useful functions. I created multiple issues to track that: issues listed in the PR description.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner mentioned this pull request Nov 13, 2023
Scope of the limited API capi-workgroup/api-evolution#41
Open
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@scoder @aisk
pythongh-106320: Re-add some PyLong/PyDict C-API functions (GH-python…
0956570
…#111162)

* pythongh-106320: Re-add _PyLong_FromByteArray(), _PyLong_AsByteArray() and _PyLong_GCD() to the public header files since they are used by third-party packages and there is no efficient replacement.

See python#111140
See python#111139

* pythongh-111262: Re-add _PyDict_Pop() to have a C-API until a new public one is designed.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@scoder @Glyphack
pythongh-106320: Re-add some PyLong/PyDict C-API functions (GH-python…
4aa7b76
…#111162)

* pythongh-106320: Re-add _PyLong_FromByteArray(), _PyLong_AsByteArray() and _PyLong_GCD() to the public header files since they are used by third-party packages and there is no efficient replacement.

See python#111140
See python#111139

* pythongh-111262: Re-add _PyDict_Pop() to have a C-API until a new public one is designed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou approved these changes
@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner
@vstinner vstinner Awaiting requested review from vstinner
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@scoder @vstinner @encukou
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.