Cython3 migration #200
Cython3 migration #200
Conversation
|
Warning Rate limit exceeded@timosachsenberg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRaises minimums to Python ≥ 3.9 and Cython ≥ 3.0, adds runtime Cython version check, removes legacy Cython-version compatibility code, updates test fixtures to language_level=3 and Python3 syntax, adjusts CI matrix, and updates docs and dev setup instructions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
autowrap/PXDParser.py (1)
406-416: I'll help you verify the concerns raised in this review comment. Let me start by examining the relevant code sections and their context.
<function_calls>
First, let's check the structure of the file and find the relevant sections
wc -l autowrap/PXDParser.py
</function_calls>Let me read the sections mentioned in the review:
<function_calls>
Read lines 406-420 to see the template parameter handling
sed -n '406,420p' autowrap/PXDParser.py
</function_calls>Now let's check the
parse_pxd_filefunction and the problematicsys.version_info.majorusage:
<function_calls>
Read lines 620-635 to see the parse_pxd_file function and language_level assignment
sed -n '620,635p' autowrap/PXDParser.py
</function_calls>Let me check the imports at the top of the file:
<function_calls>
Check the first 50 lines for imports
head -50 autowrap/PXDParser.py | grep -n "import"
</function_calls>Now let me search for where
sysis imported in the entire file:
<function_calls>
Search for sys imports throughout the file
rg -n "import sys" autowrap/PXDParser.py
</function_calls>Let me check if
parse_pxd_fileis used elsewhere in the codebase:
<function_calls>
Search for calls to parse_pxd_file in the codebase
rg -n "parse_pxd_file" --type py -A 2 -B 2
</function_calls>Let me check the context around where
parse_pxd_fileis defined to understand its full scope:
<function_calls>
Find the parse_pxd_file function definition
rg -n "def parse_pxd_file" autowrap/PXDParser.py -A 30
</function_calls>Now let me check if there's any test file that might show how
parse_pxd_fileis expected to be called:
<function_calls>
Search for test files that might use parse_pxd_file
fd -e py test | head -10
</function_calls><function_calls>
Look for test references to parse_pxd_file or PXDParser
find . -name "test" -type f | head -20
</function_calls>Let me check the documentation and setup to understand the project's targeting:
<function_calls>
Check README or setup files for Python version requirements
cat README.md 2>/dev/null | head -100 || echo "No README found"
</function_calls><function_calls>
Check setup.py or pyproject.toml for version requirements
ls -la | grep -E "setup|pyproject|requirements"
</function_calls><function_calls>
Look at the main script section to see what main does
sed -n '/if name == "main":/,/^$/p' autowrap/PXDParser.py | head -50
</function_calls><function_calls>
Check how parse_pxd_file is actually being called - full function and its context
rg -n "def parse_pxd_file" autowrap/PXDParser.py -B 2 -A 50 | head -80
</function_calls><function_calls>
Let me read the exact file around lines 600-650 to see the full parse_pxd_file
sed -n '600,650p' autowrap/PXDParser.py
</function_calls><function_calls>
Let's check what's actually at line 625-627 and around it
sed -n '615,635p' autowrap/PXDParser.py | cat -n
</function_calls>Now let me check if there are any imports at the module level that might bring in
sys:
<function_calls>
Check the full top-level imports section
head -100 autowrap/PXDParser.py | cat -n
</function_calls>Let me try a more direct approach with Python to read and analyze the file:
<function_calls>
python3 << 'EOF'Read the PXDParser.py file and check the relevant sections
with open('autowrap/PXDParser.py', 'r') as f:
lines = f.readlines()Check lines 406-416
print("=== Lines 406-416 ===")
for i in range(405, min(416, len(lines))):
print(f"{i+1}: {lines[i]}", end='')print("\n=== Lines 620-635 ===")
for i in range(619, min(635, len(lines))):
print(f"{i+1}: {lines[i]}", end='')Check if sys is imported at module level
print("\n=== Checking for module-level 'import sys' ===")
module_level_sys = False
for i in range(0, min(100, len(lines))):
if 'import sys' in lines[i] and not lines[i].strip().startswith('#'):
print(f"Found at line {i+1}: {lines[i]}", end='')
module_level_sys = True
breakif not module_level_sys:
print("No module-level 'import sys' found in first 100 lines")EOF
</function_calls>Perfect! Now let me verify where
parse_pxd_fileis used in the codebase:
<function_calls>
Search for all calls to parse_pxd_file
rg "parse_pxd_file(" --type py -B 2 -A 3
</function_calls>Let me check if
sysis imported anywhere in the__main__section or elsewhere:
<function_calls>
python3 << 'EOF'Read the PXDParser.py file and find all import statements and their context
with open('autowrap/PXDParser.py', 'r') as f:
lines = f.readlines()print("=== All 'import sys' occurrences ===")
for i, line in enumerate(lines):
if 'import sys' in line:
print(f"Line {i+1}: {line.rstrip()}")
# Show context: 3 lines before and after
print(" Context:")
for j in range(max(0, i-3), min(len(lines), i+4)):
print(f" {j+1}: {lines[j].rstrip()}")
print()print("\n=== parse_pxd_file function definition ===")
for i, line in enumerate(lines):
if 'def parse_pxd_file' in line:
# Print the function and next 40 lines
for j in range(i, min(len(lines), i+40)):
print(f"{j+1}: {lines[j]}", end='')
break
EOF
</function_calls>Perfect! I now have all the information needed to verify the review comment. Let me produce the final rewritten version:
sysimport missing forsys.version_info.majorusage—hard-codelanguage_level = 3insteadTwo points in this area:
- The new
Context.from_options(options)call is appropriate for a Cython 3-only baseline and simplifies the older version-branching logic.- However,
parse_pxd_fileusesoptions.language_level = sys.version_info.majorwithout a module-levelimport sys. Sincesysis only imported within the__main__block, callingparse_pxd_filefrom other modules will raiseNameError. Additionally, this implicitly changes behavior if Python's major version becomes 4, whereas your tests and code clearly target language_level 3 semantics.Given you now hard-require Python 3 and Cython 3, pinning the language level explicitly removes the
sysdependency and future-proofs the code:options.include_path = [data] - options.language_level = sys.version_info.major + # We target Cython's Python 3 semantics regardless of the host interpreter. + options.language_level = 3This fixes the
NameErrorrisk and eliminates version introspection that is unnecessary given your fixed baseline.Also applies to: line 627
autowrap/Main.py (1)
172-186: Based on my verification, the review comment's concerns are valid and well-founded. Cython'sget_directive_defaults()indeed returns a shared, module-level dictionary, and Cython's own codebase explicitly uses defensive copying (dict(get_directive_defaults())) to avoid process-wide side effects. Autowrap currently mutates the shared dict directly, which could cause issues in multi-call scenarios.
Copy directive defaults before mutation to avoid process-wide side effects
Cython's
get_directive_defaults()returns a shared, module-level dictionary; mutating it in place will affect global defaults across the process. Cython's own compiler codebase explicitly copies the defaults before customization. Apply the same defensive pattern here:- directive_defaults = CythonOptions.get_directive_defaults() + directive_defaults = dict(CythonOptions.get_directive_defaults())Additionally,
directive_defaults["language_level"] = sys.version_info.majorwill automatically adopt Python 4 semantics when autowrap runs on Python 4. If the intent is to maintain Python 3 semantics, consider hard‑coding3instead; otherwise, document that this behavior change is intentional.
🧹 Nitpick comments (3)
autowrap/__init__.py (1)
43-56: Tighten Cython version guard and avoid silent failuresThe new runtime check is good, but two details are brittle:
- On
ImportErroryou silently continue with_cython_version = "0", which meansautowrapwill import without Cython and only fail later, contrary to the stated hard dependency.- The broad
except Exception: passhides all parsing issues and defeats the purpose of the guard, and static analysis is already flagging this.Consider something along these lines:
-try: - from Cython.Compiler.Version import version as _cython_version -except ImportError: - _cython_version = "0" -else: - try: - _major_str = str(_cython_version).split(".")[0] - if int(_major_str) < 3: - raise RuntimeError( - "autowrap requires Cython >= 3.0; found Cython %s" % _cython_version - ) - except Exception: - # If parsing the version fails for some reason, do not block import here. - pass +try: + from Cython.Compiler.Version import version as _cython_version +except ImportError as exc: + raise RuntimeError("autowrap requires Cython >= 3.0; Cython is not importable") from exc +else: + try: + _major_str = str(_cython_version).split(".")[0] + if int(_major_str) < 3: + raise RuntimeError( + "autowrap requires Cython >= 3.0; found Cython %s" % _cython_version + ) + except (TypeError, ValueError) as exc: + logger.warning( + "Failed to parse Cython version %r; skipping runtime version check", + _cython_version, + exc_info=exc, + )This keeps the guard strict where it matters (missing/too-old Cython) while avoiding a blanket
except Exception.tests/test_cython_version.py (1)
1-7: Consider centralizing Cython version parsing to avoid driftThe test correctly enforces a major Cython version ≥ 3, but it duplicates the parsing logic from
autowrap/__init__.py. To keep things in sync when the minimum version changes, consider moving the “parse Cython major version” logic into a tiny helper inautowrapand reusing it here (or vice versa) so there’s a single source of truth.MIGRATION_PLAN.MD (1)
18-23: Align migration-plan narrative with the current code and avoid brittle line referencesSections 2 and 3 still read as if the old compatibility code is “current” (e.g., the try/except around importing
directive_defaultsinautowrap/Main.py:176–188), and they talk about droppingget_directive_defaults(), while the updatedrun_cythonnow unconditionally usesCythonOptions.get_directive_defaults()without that compatibility layer.To keep this plan useful going forward, consider:
- Rephrasing these bullets to clearly distinguish historical behavior from the new Cython‑3‑only behavior (e.g., “Previously…, this PR now simplifies to…”), and reflect that the chosen approach is
get_directive_defaults().- Removing or softening the hard-coded
file:line–linereferences, which will quickly go stale as the code evolves (a short description of the area or function name is usually enough).This keeps the migration plan accurate and easier to maintain as the implementation continues to evolve.
Also applies to: 38-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
CHANGELOG.md(1 hunks)MIGRATION_PLAN.MD(1 hunks)README.md(1 hunks)README_DEVELOP(1 hunks)TODO(1 hunks)autowrap/Main.py(1 hunks)autowrap/PXDParser.py(2 hunks)autowrap/__init__.py(1 hunks)docs/README.md(2 hunks)pyproject.toml(1 hunks)tests/test_code_generator.py(0 hunks)tests/test_cython_version.py(1 hunks)tests/test_files/A.pxd(1 hunks)tests/test_files/B.pxd(1 hunks)tests/test_files/C.pxd(1 hunks)tests/test_files/Cycle0.pxd(1 hunks)tests/test_files/Cycle1.pxd(1 hunks)tests/test_files/Cycle2.pxd(1 hunks)tests/test_files/D.pxd(1 hunks)tests/test_files/base.pxd(1 hunks)tests/test_files/base0.pxd(1 hunks)tests/test_files/enums.pxd(2 hunks)tests/test_files/gil_testing.pxd(1 hunks)tests/test_files/inherited.pxd(2 hunks)tests/test_files/int_container_class.pxd(1 hunks)tests/test_files/int_container_class.pyx(1 hunks)tests/test_files/itertest.pyx(2 hunks)tests/test_files/libcpp_stl_test.pxd(1 hunks)tests/test_files/libcpp_test.pxd(1 hunks)tests/test_files/libcpp_utf8_output_string_test.pxd(1 hunks)tests/test_files/libcpp_utf8_string_test.pxd(1 hunks)tests/test_files/minimal.pxd(1 hunks)tests/test_files/minimal_td.pxd(1 hunks)tests/test_files/number_conv.pxd(1 hunks)tests/test_files/templated.pxd(1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_code_generator.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-13T12:43:19.355Z
Learnt from: pjones
Repo: OpenMS/autowrap PR: 198
File: pyproject.toml:2-2
Timestamp: 2025-08-13T12:43:19.355Z
Learning: For PEP 639 compatibility, the documentation explicitly specifies setuptools version 77.0.3 as the minimum requirement, not 77.0.0.
Applied to files:
pyproject.toml
📚 Learning: 2025-08-13T12:43:19.355Z
Learnt from: pjones
Repo: OpenMS/autowrap PR: 198
File: pyproject.toml:2-2
Timestamp: 2025-08-13T12:43:19.355Z
Learning: When implementing PEP 639 compatibility, the user pjones has indicated that specific documentation explicitly requires setuptools version 77.0.3, not just ≥77.0, though the general sources indicate ≥77.0 should be sufficient.
Applied to files:
pyproject.toml
🪛 LanguageTool
MIGRATION_PLAN.MD
[grammar] ~31-~31: Use a hyphen to join words.
Context: ...options)(or the canonical Cython 3 way to construct aContext`). - Keep or...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.6)
autowrap/__init__.py
51-53: Abstract raise to an inner function
(TRY301)
54-56: try-except-pass detected, consider logging the exception
(S110)
54-54: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (29)
tests/test_files/inherited.pxd (1)
1-1: LGTM! Language level update is correct.The language_level directive has been correctly updated to 3 as part of the Cython 3+ migration.
tests/test_files/libcpp_test.pxd (1)
1-1: LGTM! Language level update is correct.The language_level directive has been correctly updated to 3 as part of the Cython 3+ migration.
tests/test_files/int_container_class.pyx (1)
1-1: LGTM! Language level update is correct.The language_level directive has been correctly updated to 3 as part of the Cython 3+ migration.
tests/test_files/C.pxd (1)
1-1: LGTM! Language level update is correct.The language_level directive has been correctly updated to 3 as part of the Cython 3+ migration.
tests/test_files/gil_testing.pxd (1)
1-1: LGTM! Language level update is correct.The language_level directive has been correctly updated to 3 as part of the Cython 3+ migration.
tests/test_files/libcpp_utf8_string_test.pxd (1)
1-1: LGTM! Language level update is correct.The language_level directive has been correctly updated to 3 as part of the Cython 3+ migration.
tests/test_files/int_container_class.pxd (1)
1-1: LGTM! Language level update is correct.The language_level directive has been correctly updated to 3 as part of the Cython 3+ migration.
tests/test_files/D.pxd (1)
1-1: LGTM! Language level update is correct.The language_level directive has been correctly updated to 3 as part of the Cython 3+ migration.
tests/test_files/enums.pxd (1)
1-1: Enums test migrated cleanly to language_level=3Header directive and enum block remain consistent with intended C++ mapping; no behavioral change beyond Python 3 semantics for this test file.
Also applies to: 16-25
tests/test_files/Cycle1.pxd (1)
1-1: Cycle1.pxd language_level bump is consistentOnly the Cython directive changed; extern/cppclass definitions are untouched and should behave identically under language_level=3.
tests/test_files/minimal_td.pxd (1)
1-1: Minimal typedef test updated to language_level=3Mechanical directive update; the C typedef remains the same and should compile as before.
tests/test_files/base.pxd (1)
1-1: Base template test aligned with Cython 3 language_levelDirective bump only; templated cppclass and wrap comments are unchanged.
tests/test_files/B.pxd (1)
1-1: B.pxd directive update matches the migration patternOnly the language_level changes; cppclass and wrap metadata remain intact.
tests/test_files/libcpp_utf8_output_string_test.pxd (1)
1-1: UTF‑8 output string test moved to language_level=3Mechanical header change; libcpp string alias and cppclass interface are unchanged.
tests/test_files/itertest.pyx (1)
1-1: Iterator test correctly updated to Cython 3 / Python 3 syntaxSwitching to language_level=3 and using
print("dealloc called")keeps the test behavior while aligning with Python 3 semantics.Also applies to: 14-15
tests/test_files/libcpp_stl_test.pxd (1)
1-1: STL test header migrated to language_level=3 without API changesThe many cppclass and method declarations remain identical; only the Cython language_level is updated to match the new baseline.
tests/test_files/minimal.pxd (1)
1-1: Language level 3 directive is consistent and safe hereThis file only declares C++ interop types/functions, so switching to
language_level=3is compatible and aligns with the new Cython ≥ 3 baseline.tests/test_files/A.pxd (1)
1-1: Header updated to Cython 3 language level appropriatelyThe
language_level=3directive matches the project-wide Cython 3+ requirement and is safe given the purely C++ declarations.tests/test_files/templated.pxd (1)
1-1: Templated test header correctly moved to language_level=3No Python 2 semantics are present, so adopting
language_level=3here cleanly matches the Cython 3+ migration.tests/test_files/base0.pxd (1)
1-1: Base0.pxd header update aligns with Cython 3 requirementThe move to
language_level=3is appropriate for this simple extern/cppclass declaration file.tests/test_files/number_conv.pxd (1)
1-1: Number conversion test now uses Cython language_level=3Given this file only exposes C/C++ declarations, the
language_level=3change is low-risk and consistent with the rest of the suite.tests/test_files/Cycle2.pxd (1)
1-1: Cycle2.pxd language level updated appropriatelyThe directive bump to
language_level=3is in line with the Cython 3 migration and has no effect on the extern cppclass definition.tests/test_files/Cycle0.pxd (1)
1-1: Cycle0.pxd header matches new Cython 3 baselineUpdating the directive to
language_level=3keeps this cyclic-inheritance test file aligned with the rest of the suite.README_DEVELOP (1)
1-33: Dev setup instructions look good; clean up the trailing orphaned lineThe new virtualenv +
pip install -e .[dev]workflow and test/CLI commands are clear and aligned with the currentpyproject.toml.There’s a leftover fragment at Line 34 (
from the projects directory.) that no longer has context and can be removed or folded into the preceding paragraph to avoid confusion.A minimal fix would be:
-These commands have been verified to work with the current `pyproject.toml` -configuration and are the preferred starting point for developing on `autowrap`. - -from the projects directory. +These commands have been verified to work with the current `pyproject.toml` +configuration and are the preferred starting point for developing on `autowrap`.Likely an incorrect or invalid review comment.
TODO (1)
1-36: TODO updates are clear and aligned with the new Cython 3+ baselineThe refactor notes (CodeGenerator split, iterator support, reference holders) are well-scoped and consistent with the migration direction; nothing blocking here.
pyproject.toml (1)
32-32: Cython dependency bump is consistent with the migrationSetting
Cython>=3.0here matches the new runtime guard and documentation, andsetuptools >= 77.0.3aligns with the earlier PEP 639 requirement. Just ensure any downstream tooling that previously relied on older Cython is updated accordingly.README.md (1)
17-22: Requirements section looks good and consistentThe documented requirements (Python ≥ 3.9, Cython ≥ 3.0) are in line with pyproject, CHANGELOG, and tests.
CHANGELOG.md (1)
5-5: Changelog entry correctly captures the compatibility changeThe note about requiring Cython ≥ 3.0 and Python ≥ 3.9 and dropping older Cython versions matches the actual code, tests, and config changes in this PR.
docs/README.md (1)
12-17: Docs updates align with the new baseline and Python 3 syntaxThe added Requirements section (Python ≥ 3.9, Cython ≥ 3.0) and the switch to
print(...)in the interactive example bring this doc in line with the rest of the project and current Python practices.Also applies to: 92-98
|
@timosachsenberg I've opened a new pull request, #203, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Remove MIGRATION_PLAN.MD as requested Co-authored-by: timosachsenberg <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: timosachsenberg <[email protected]>
Removed incompatible Cython and Python version combinations.
E NameError: name 'cython_version' is not defined remove this in tests/test_code_generator.py::test_enums it is probably not necessary anymore |
|
@timosachsenberg I've opened a new pull request, #206, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Remove obsolete Cython version check in test_enums Co-authored-by: timosachsenberg <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: timosachsenberg <[email protected]>
This pull request drops support for legacy Cython versions and updates the codebase, tests, and documentation to require Cython ≥ 3.0 and Python ≥ 3.9. It removes compatibility code for older Cython APIs, modernizes example and test files to use Python 3 syntax and semantics, and adds clear migration and setup instructions for developers. The changes ensure that autowrap consistently targets Cython 3+ and Python 3+ throughout its code, tests, and documentation.
Compatibility and Requirements
pyproject.toml, documentation (README.md,docs/README.md), and changelog to require Python ≥ 3.9 and Cython ≥ 3.0, and removed references to older Cython versions. [1] [2] [3] [4]autowrap/__init__.pyto raise an error if Cython < 3 is detected.Codebase Modernization
autowrap/PXDParser.pyandautowrap/Main.py, now assuming Cython 3+ APIs throughout. [1] [2] [3]Test Suite Updates
.pxdtest files to uselanguage_level=3instead of2. [1] [2] [3] [4] [5] [6] [7] [8]Documentation and Developer Experience
README_DEVELOPto use virtual environments andpip install -e .[dev].print()function).Internal Planning and Refactoring
TODOfile to outline plans for internal refactoring, improved iterator and reference support, and safer wrapping practices, aligning with the new Cython 3+ baseline.Summary by CodeRabbit
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.