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

Add support for Python 3.13 #241

Merged
merged 11 commits into from
Jan 22, 2025
Merged

Add support for Python 3.13 #241

merged 11 commits into from
Jan 22, 2025

Conversation

cbrnr
Copy link
Owner

@cbrnr cbrnr commented Oct 9, 2024

In addition, bump dependencies.

Copy link
Owner Author

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

NumPy errors are all in wfdb, I'll try to fix them (will link the corresponding PR once it's up).

@cbrnr cbrnr linked an issue Oct 14, 2024 that may be closed by this pull request
3 tasks
Copy link
Owner Author

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

We have to wait for MIT-LCP/wfdb-python#511 (and a new release).

@cbrnr
Copy link
Owner Author

cbrnr commented Jan 22, 2025

This seems to work nicely. However, I don't like that some dependencies are installed in cibuildwheel.yml since everything should already be defined in pyproject.toml. Specifically, we created the cibw extras group exactly for this purpose. @larsoner do you know how the "Triage dependencies" step can be modified to use this extras group instead of manually installing packages?

@larsoner
Copy link
Contributor

In theory you could modify the python -m pip install ./dist/*.whl command, probably first by getting the wheel filename then using the file:// syntax to get to something like pip install file://$WHEEL_PATH[some_new_option] and add some_new_option to pyproject.toml or whatever. TBD whether or not pip works properly with a file:// plus optional specifier, though!

@cbrnr
Copy link
Owner Author

cbrnr commented Jan 22, 2025

I wonder why you switched to running pytest manually, since previously we used cibuildwheel functionality to run our tests (e.g., test-requires and test-command in the [tool.cibuildwheel] section, see https://cibuildwheel.pypa.io/en/stable/options/). Does this not work with your changes anymore?

@larsoner
Copy link
Contributor

I wonder why you switched to running pytest manually, since previously we used cibuildwheel functionality to run our tests

FWIW it should still run tests in cibuildwheel as before. But now it does more than that.

If the cibuildwheel tests are all you had, it would only build and test on 3.9 (or 3.10 or whatever your min version is). So if you want to test multiple configs like different Python versions (e.g., 3.13, 3.11) or different dependency groupings, etc. you want a separate run. Having tests in a separate run also has the really nice side benefit of creating a clean setup to test from that doesn't have any of the stuff installed from the cibuildwheel job, so you can be more confident that things will install and run properly once deployed in the wild.

@cbrnr
Copy link
Owner Author

cbrnr commented Jan 22, 2025

Ah, you're right, the cibuildwheel tests are still there, but they only test the wheel they've just built (i.e., using Python 3.10 currently).

It's still a bit of duplication going on because of the additional tests, which is not ideal. I'll try and see if dropping cibuildwheel (and instead building and testing wheels on runners using uv) simplifies things in a future PR.

@cbrnr cbrnr merged commit 40d9067 into main Jan 22, 2025
15 checks passed
@cbrnr cbrnr deleted the bump branch January 22, 2025 14:30
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.

Python 3.13 roadmap
2 participants