-
Notifications
You must be signed in to change notification settings - Fork 28
Update CI to build for Python 3.13 and linux-aarch64 #101
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
Conversation
|
@apcamargo thanks for taking this on! Do you have any insights on the overhead impact of the mutex? Maybe it's worth benchmarking this against other approaches (I'm not sure if Atomics is suitable here, but it might be worth looking into). Also, do you know if free-threading support is required for building a 3.13 wheel? If not, it might be best to split this into two PRs. |
|
It seems that starting from PyO3 0.23.0 we will have to take free-threading into account. I think it's best to not postpone, since we will have to update PyO3 anyway. I'll do some benchmarking and get back to you! |
|
@audy I gave it a try, but I couldn't figure out how to use atomics here. I also ran a quick benchmark reading a large FASTA file, comparing this branch to master, and the performance difference was negligible. For reference, here is the error I get when trying to compile without changing anything in the function: |
audy
left a comment
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.
Ran a simple benchmark of the Python bindings and also saw no significant impact on performance.
This is good to go @apcamargo. Do you mind resolving the merge conflict? Then, I will release a new version to PyPI.
Thanks :)
|
Thanks, @audy! Before pushing a new release to PyPI, I wanted to make some additional changes. These changes were originally intended for a separate pull request, but I forgot to create a branch before committing 😅 Could you review and confirm if you’re ok with these changes?
|
|
@apcamargo yes, those would be excellent additions |
|
Ok then. Those are already committed, só I think we can merge :) |
audy
left a comment
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.
Awesome. Thanks @apcamargo


This PR:
FastxReader.PyFastxReaderasFastxReaderin the Python module.