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

build: support Python 3.13 #1263

Merged
merged 5 commits into from
Dec 29, 2024
Merged

build: support Python 3.13 #1263

merged 5 commits into from
Dec 29, 2024

Conversation

shiftinv
Copy link
Member

@shiftinv shiftinv commented Dec 29, 2024

Summary

This adds the 3.13 package classifier, and updates dependencies where needed (see below). Unlike 3.12, 3.13 doesn't really require a whole lot of code changes.

CI

This does not add 3.13 to the CI matrix, as some dependency updates, namely pycares (through aiodns) and libcst, have already dropped Python 3.8 support in the versions that add prebuilt 3.13 wheels, which results in PDM refusing to resolve dependencies.
While the current dependency versions technically all run fine on 3.13, the lack of prebuilt wheels means each CI job would spend a solid minute just building libcst and pycares, which should definitely not be necessary.
Once we update PDM, https://pdm-project.org/en/latest/usage/lock-targets/ can likely solve this.

audioop

3.13 removed audioop, which is used in PCMVolumeTransformer.
Re-implementing this in pure Python is certainly doable, but given that it's somewhat performance-critical and ultimately just number crunching, even an optimized implementation is not particularly fast compared to the native implementation.
To put this into perspective, have some (rough) pyperf benchmarks of a couple different attempts:

native: Mean +- std dev: 9.28 us +- 0.36 us
new_thingy: Mean +- std dev: 352 us +- 4 us
new_thingy_array: Mean +- std dev: 259 us +- 4 us
new_thingy_dict_lookup: Mean +- std dev: 232 us +- 6 us
new_thingy_array_lookup: Mean +- std dev: 124 us +- 2 us
new_thingy_bytes_lookup: Mean +- std dev: 83.6 us +- 1.8 us
`bytes_lookup`
shortstruct = struct.Struct("<h")  # signed short LE

# precomputed lookup table for one specific factor
bytes_lookup = []
for i in range(65536):
    v = int(factor * (i - 32768))
    v = 32767 if v > 32767 else (-32768 if v < -32768 else v)
    bytes_lookup.append(shortstruct.pack(v))

def new_thingy_bytes_lookup(fragment: bytes, width: int, factor: float):
    a = array.array("h", fragment)
    return b"".join([bytes_lookup[s + 32768] for s in a])

Long story short, disnake[voice] now adds audioop-lts on 3.13, see 11e87cc for rationale. In the long term, I would prefer getting rid of audioop entirely by no longer shipping PCMVolumeTransformer in the library.

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running pdm lint
    • I have type-checked the code by running pdm pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

I don't love using an entirely separate dependency just for a single function
that purely does number crunching, but for this same reason it can't just
be reimplemented in pure Python. I'll probably but some benchmarks in the PR
this ends up in, but as a general idea, even the most optimized pure Python
implementations can barely reach 8-10% of the speed the native one achieves.

Since this function needs to run once per PCM frame, i.e. every 20ms,
performance is fairly critical here. While getting a pure Python implementation
to run within those 20ms is extremely trivial with even just a basic
reimplementation, multiple parallel audio players would have a non-negligible
perf overhead.
@shiftinv shiftinv added t: dependencies Addition/update/removal of dependencies t: meta Changes to the project itself (CI, configs, etc.) labels Dec 29, 2024
@shiftinv shiftinv added this to the disnake v2.10 milestone Dec 29, 2024
@shiftinv shiftinv requested a review from a team as a code owner December 29, 2024 14:21
@shiftinv shiftinv merged commit 6bcecb2 into master Dec 29, 2024
29 checks passed
@shiftinv shiftinv deleted the feat/py313 branch December 29, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: dependencies Addition/update/removal of dependencies t: meta Changes to the project itself (CI, configs, etc.)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant