Conversation
Add `from __future__ import annotations` to three files that use PEP 604 union type syntax (`X | Y`), which requires Python 3.10+ at runtime but works on 3.7+ with deferred evaluation. Also add Python 3.9 to the toolchain compatibility CI matrix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…strap gate from 3.10 to 3.9 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude Code ReviewHead SHA: fe3fb26 Files changed: 6
Summary
Findings1. Incomplete coverage of The PR adds the future import to 4 files, but there may be other files in the toolchain that also use Recommend running a grep before merging to confirm no other files need the same treatment: grep -rn '|' toolchain/ docs/ --include='*.py' | grep -E 'def |: ' | grep -v '__future__'Or more precisely: grep -rn ' | None\b\|: \w\+ | \w\+' toolchain/ docs/ --include='*.py'2. Minor:
No blocking issues foundThe approach is correct: |
Review Summary by QodoSupport Python 3.9 with deferred annotation evaluation
WalkthroughsDescription• Add from __future__ import annotations to 4 Python files for PEP 604 union syntax support • Lower minimum Python version requirement from 3.10 to 3.9 • Add Python 3.9 to CI toolchain compatibility test matrix Diagramflowchart LR
A["Python 3.9 Support"] --> B["Add future annotations<br/>to 4 files"]
A --> C["Lower bootstrap<br/>gate to 3.9"]
A --> D["Update CI matrix<br/>to include 3.9"]
B --> E["Enable PEP 604<br/>union syntax"]
File Changes1. docs/postprocess_citations.py
|
Code Review by Qodo
1. Automation docs still say 3.10+
|
Claude Code ReviewHead SHA: fe3fb26 Files changed: 6
Summary:
Findings: No correctness issues found. The approach is sound: Improvement opportunities:
Overall: The changes are minimal, correct, and well-scoped. Approve. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request extends Python 3.9 support across the codebase. The minimum Python version requirement is lowered from 3.10 to 3.9 in the bootstrap configuration, and the GitHub Actions test matrix is updated to include Python 3.9 testing alongside versions 3.10 through 3.14. Additionally, multiple Python source files are updated to include postponed evaluation of type annotations through future imports, ensuring consistency and forward compatibility with Python's typing system. 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 |
There was a problem hiding this comment.
Pull request overview
This PR aims to extend the Python toolchain/aux scripts to run on Python 3.9 by adjusting version gating, updating CI, and attempting to address Python 3.10-only typing syntax usage.
Changes:
- Add
from __future__ import annotationsto four Python modules/scripts. - Lower the bootstrap minimum Python version from 3.10 to 3.9.
- Expand the toolchain compatibility CI matrix to include Python 3.9.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
toolchain/mfc/params/generators/docs_gen.py |
Adds postponed-annotations future import to support typing usage. |
toolchain/mfc/params/ast_analyzer.py |
Adds postponed-annotations future import to support typing usage. |
toolchain/mfc/gen_case_constraints_docs.py |
Adds postponed-annotations future import to support typing usage. |
toolchain/bootstrap/python.sh |
Lowers the bootstrap Python version gate to 3.9. |
docs/postprocess_citations.py |
Adds postponed-annotations future import to support typing usage. |
.github/workflows/test-toolchain-compat.yml |
Adds Python 3.9 to the CI compatibility matrix. |
Comments suppressed due to low confidence (2)
toolchain/mfc/params/ast_analyzer.py:16
- Adding
from __future__ import annotationsdoes not makeX | Ytype annotations compatible with Python 3.9 (they are a 3.10 grammar feature). This file still usesstr | None(e.g.,self.current_method: str | None), which will be aSyntaxErroron 3.9. Replace these withOptional[str]/Union[...](or string-literal annotations) to truly support 3.9.
from __future__ import annotations
import ast
import re
from dataclasses import dataclass, field
from pathlib import Path
from typing import Dict, List, Optional, Set
toolchain/mfc/gen_case_constraints_docs.py:18
from __future__ import annotationsdoes not allow PEP 604 union syntax (X | Y) on Python 3.9; the parser will still raiseSyntaxError. This script still declares unions likeint | Nonein type annotations, so lowering the minimum Python version won’t work until those are rewritten (e.g.,Optional[int]).
from __future__ import annotations
import json
import sys
import subprocess
from dataclasses import dataclass
from pathlib import Path
from typing import Dict, List, Iterable, Any
| from __future__ import annotations | ||
|
|
||
| import re | ||
| import sys | ||
| from pathlib import Path |
There was a problem hiding this comment.
Python 3.9 cannot parse PEP 604 union type syntax (X | Y) even with from __future__ import annotations. This module still contains annotations like tuple[str, int, str | None], which will be a SyntaxError on 3.9. Replace these unions with Optional[...]/Union[...] (or quote the annotation expressions) if 3.9 support is intended.
| MFC_PYTHON_MIN_MAJOR=3 | ||
| MFC_PYTHON_MIN_MINOR=10 | ||
| MFC_PYTHON_MIN_MINOR=9 | ||
| MFC_PYTHON_MIN_STR="$MFC_PYTHON_MIN_MAJOR.$MFC_PYTHON_MIN_MINOR" |
There was a problem hiding this comment.
Lowering the bootstrap minimum to Python 3.9 conflicts with the current codebase: several modules still contain PEP 604 union annotations (X | Y), which are a Python 3.10+ syntax feature and will raise SyntaxError on 3.9 regardless of from __future__ import annotations. Either keep the minimum at 3.10, or fully migrate those annotations to typing.Optional/typing.Union (and verify dependencies support 3.9) before lowering this gate.
| fail-fast: false | ||
| matrix: | ||
| python-version: ['3.10', '3.11', '3.12', '3.13', '3.14'] | ||
| python-version: ['3.9', '3.10', '3.11', '3.12', '3.13', '3.14'] |
There was a problem hiding this comment.
Adding Python 3.9 to the CI matrix will currently fail because parts of the codebase still use Python 3.10-only syntax in type annotations (X | Y). Either land the annotation migration in the same PR (so the job passes), or keep 3.9 out of the matrix until the code is actually parseable on 3.9.
| python-version: ['3.9', '3.10', '3.11', '3.12', '3.13', '3.14'] | |
| python-version: ['3.10', '3.11', '3.12', '3.13', '3.14'] |
| from __future__ import annotations | ||
|
|
There was a problem hiding this comment.
from __future__ import annotations does not make PEP 604 union syntax (X | Y) parse on Python 3.9; it only changes annotation evaluation. This file still contains a Dict[str, list] | None annotation, which will raise SyntaxError on 3.9. To actually support 3.9, rewrite these unions using typing.Optional/typing.Union (or quote the annotation expressions), and then drop the future import if it’s no longer needed.
| from __future__ import annotations |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1287 +/- ##
=======================================
Coverage 44.95% 44.95%
=======================================
Files 70 70
Lines 20503 20503
Branches 1946 1946
=======================================
Hits 9217 9217
Misses 10164 10164
Partials 1122 1122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Extend AMD-only VLA guards to also cover CCE 19.0.0 in all four locations in m_chemistry.fpp: - s_compute_q_T_sf: add USING_CCE guard for Ys array - s_compute_T_from_primitives: add USING_CCE guard for Ys array - s_compute_chemistry_reaction_flux: extend AMD guard to include CCE - s_compute_chemistry_diffusion_flux: extend AMD guard to include CCE CCE 19.0.0 LLVM InstCombine (foldIntegerTypedPHI) crashes on VLAs (dimension(num_species)) when num_species is not a compile-time constant (e.g., case-optimized pre_process for non-chemistry cases). Fixed by using dimension(10) when USING_CCE. Also restore MFC_PYTHON_MIN_MINOR=9 in python.sh (was accidentally set to 10 in 3d8cf31; PR MFlowCode#1287 merged to master adds Python 3.9 support). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
from __future__ import annotationsto 4 files using PEP 604 union type syntax (X | Y), which requires Python 3.10+ at runtime but works on 3.7+ with deferred evaluationTest plan
./mfc.sh lintpasses on Python 3.9.21./mfc.sh precheckpasses on Python 3.9.21