-
Notifications
You must be signed in to change notification settings - Fork 31
MAINT: Baseline setup for free-threading #49
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
|
Subsumed the typo PR #48 into this one |
rgommers
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.
Thanks @HaoZeke. Looks largely right. A few small comments.
Also note that the cibuildwheel version needs bumping to the latest released version or the enable = part won't work, here:
cymem/.github/workflows/cibuildwheel.yml
Line 22 in a66af97
| uses: pypa/[email protected] |
| self.matrices = <SparseMatrix**>self.mem.alloc(self.length, sizeof(SparseMatrix*)) | ||
| for i, py_matrix in enumerate(py_matrices): | ||
| self.matrices[i] = sparse_matrix_init(self.mem, py_matrix) | ||
| self.matrices[i] = sparse_matrix_init_cymem(self.mem, py_matrix) |
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.
It's minor, but this would have been better left as a separate PR, since it's unrelated to the topic of this PR, trivial to review/merge, and after that CI will run on this PR without approval.
rgommers
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.
This LGTM now, and the added cp313t CI jobs pass after rebasing on top of gh-51. I think that PR should be merged first, and then this one after triggering CI.
Part of the next PR...
Co-authored-by: rgommers <[email protected]>
|
gh-53 is now merged, which addresses free-threading support fully, in a similar but slightly different way. At this point, 3.13t support is no longer needed, 3.14t only is fine. So while this PR was actually ready to go at the time, it now has merge conflicts and doesn't add anything anymore. Hence I will go ahead and close this. Thanks again @HaoZeke. |
Implements (1) as suggested by @rgommers in #47 (comment). Basically builds without a warning, but isn't really safe.