Skip to content

Conversation

@apcamargo
Copy link
Contributor

@apcamargo apcamargo commented Mar 31, 2025

This PR:

  • Updates the CI to include builds for Python 3.9, 3.10, 3.13, and linux-aarch64.
  • Updates the code for compatibility with PyO3 0.24, incorporating support for free-threaded Python by using locks in FastxReader.
  • Exposes PyFastxReader as FastxReader in the Python module.
  • Adds a stub file to enable IDE type hinting and docstring display upon hovering.

@audy
Copy link
Contributor

audy commented Apr 4, 2025

@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.

@apcamargo
Copy link
Contributor Author

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!

@apcamargo
Copy link
Contributor Author

@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:

error[E0277]: `(dyn FastxReader + 'static)` cannot be shared between threads safely
   --> src/python.rs:58:12
    |
58  | pub struct PyFastxReader {
    |            ^^^^^^^^^^^^^ `(dyn FastxReader + 'static)` cannot be shared between threads safely
    |
    = help: the trait `std::marker::Sync` is not implemented for `(dyn FastxReader + 'static)`

Copy link
Contributor

@audy audy left a 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 :)

@apcamargo
Copy link
Contributor Author

apcamargo commented Apr 18, 2025

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?

  • Expose PyFastxReader as FastxReader in the Python module.
  • Add a stub file to enable IDE type hinting and docstring display upon hovering.

Before:
image

After:
image

@audy
Copy link
Contributor

audy commented Apr 18, 2025

@apcamargo yes, those would be excellent additions

@apcamargo
Copy link
Contributor Author

Ok then. Those are already committed, só I think we can merge :)

Copy link
Contributor

@audy audy left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks @apcamargo

@audy audy merged commit 3310a37 into onecodex:master Apr 18, 2025
30 checks passed
@apcamargo apcamargo deleted the python-3.13 branch April 18, 2025 22:45
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.

2 participants