MAINT: Unpin Cython version constraint for Cython >= 3.1.0 (#5057) - #5468
MAINT: Unpin Cython version constraint for Cython >= 3.1.0 (#5057)#5468AkshitBoora wants to merge 6 commits into
Conversation
Documentation build overview
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5468 +/- ##
===========================================
- Coverage 93.87% 93.63% -0.24%
===========================================
Files 182 182
Lines 22522 22522
Branches 3206 3206
===========================================
- Hits 21143 21089 -54
- Misses 917 965 +48
- Partials 462 468 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…nsion compile args (MDAnalysis#5057)
…am linetrace crashes (MDAnalysis#5057)
…on 3.1+ compatibility (MDAnalysis#5057) - Remove 'plugins = Cython.Coverage' from .coveragerc since Cython 3.1+ has worker crash bugs with linetrace which the Cython.Coverage plugin requires. Python-level coverage via pytest-cov still works. - Fix black formatting in setup.py (line-length compliance)
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
I'm sorry, I'm going to block this PR for furtther discussion since this just doesn't seem like the right way to go about fixing this issue - nor am I sure that there is an issue to actually fix here.
As far as I know, we don't want to just blanket turn off Cython coverage (at least for now).
| python-version: ["3.11", "3.12", "3.13"] | ||
| full-deps: [true, ] | ||
| codecov: [true, ] | ||
| cython: ["cython<3.1", ] |
There was a problem hiding this comment.
The testing of cython < 3.1 and cython >3.1 is intentional.
| # Cython.Coverage plugin disabled for Cython >= 3.1.0 compatibility (Issue #5057). | ||
| # Cython 3.1+ has worker crash bugs with linetrace which is required by the | ||
| # Cython.Coverage plugin, so we disable the plugin to prevent test crashes. |
There was a problem hiding this comment.
I don't believe that just disabling coverage is the solution to this issue.
| cython_linetrace = bool(os.environ.get("CYTHON_TRACE_NOGIL", False)) | ||
| if Version(Cython.__version__) >= Version("3.1.0"): | ||
| # Cython 3.1+ has worker crash bugs with linetrace (Issue #5057) | ||
| cython_linetrace = False |
There was a problem hiding this comment.
This would change the tracing pytest behavior with Cython 3.1+ from a deadlock to a silent failure to perform the coverage tracing, which I'm not sure is desirable. One could even argue for a "fail fast and early" design here, where you instead go in the opposite direction and refuse to build with a clear failure under the problematic circumstances.
Seems like the desired solution would be to fix the upstream issue (cython/cython#6658) and then always require a very recent version of Cython, which is generally considered quite acceptable b/c it is a build time/dev dep. For example, SciPy requires 3.2.0+ already: https://github.com/scipy/scipy/blob/main/pyproject.toml#L22
3 participants
Fixes #5057
Changes made in this Pull Request:
cython<3.1constraint across CI workflow matrix configurations (.github/workflows/gh-ci.yamland.github/workflows/gh-ci-cron.yaml).package/CHANGELOGunderFixesreferencing Issue MDAnalysis fails with Cython 3.1.0 #5057.Akshit Booratopackage/AUTHORS.LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes (assisted in workflow syntax inspection, changelog formatting, and requirement verification)
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS?Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.