fix[ci]: modernize CI and petl dependencies - #702
Conversation
PR Summary by QodoCI: switch workflow builds from setup.py to PEP 517 (python -m build)
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Coverage Report for CI Build 29780656074Warning No base build found for commit Coverage: 91.581%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Code Review by Qodo
1. Build step breaks py27
|
| - name: Setup petl package with mode = ${{ matrix.mode }} | ||
| run: python setup.py sdist bdist_wheel | ||
| run: | | ||
| python -m pip install --upgrade build setuptools wheel | ||
| python -m build |
There was a problem hiding this comment.
1. Build step breaks py27 🐞 Bug ☼ Reliability
The workflow now unconditionally installs and runs the build frontend (python -m build) in the main test matrix, which includes a Python 2.7 entry. This introduces a new packaging-tool dependency into the Python 2.7 job and can fail CI before tests run if that tool cannot be installed/executed on that interpreter.
Agent Prompt
### Issue description
The CI matrix includes Python 2.7, but the workflow now always installs the `build` frontend and runs `python -m build`. This adds a new dependency path for the py2.7 job and can prevent that job from ever reaching pytest.
### Issue Context
- The matrix explicitly includes `python: "2.7"`.
- The build step has no `if:` guard and will execute for that entry.
### Fix Focus Areas
- .github/workflows/test-changes.yml[38-75]
- .github/workflows/test-changes.yml[196-200]
### Suggested fix
- Add an `if:` guard so the `python -m pip install ... build` + `python -m build` step runs only for Python >= 3 (or `matrix.python != '2.7'`).
- For the Python 2.7 leg, either:
- keep the legacy command (`python setup.py sdist bdist_wheel`), or
- skip building entirely if it’s not needed for the test run.
- Optionally, pin build tool versions (or use a constraints file) to reduce future CI breakage.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - name: Setup petl package with mode = ${{ matrix.mode }} | ||
| run: python setup.py sdist bdist_wheel | ||
| run: | | ||
| python -m pip install --upgrade build setuptools wheel | ||
| python -m build |
There was a problem hiding this comment.
2. Tests not run from wheel 🐞 Bug ⚙ Maintainability
CI runs pytest and Sphinx against the repository checkout (not an installed wheel/editable install) even though petl/version.py is a generated (gitignored) file required for import petl. This makes the test environment depend on packaging side effects and does not verify that the built artifact is importable/behaves correctly when installed.
Agent Prompt
### Issue description
The workflow builds the package but then runs pytest and docs generation against the source tree, which relies on generated files (e.g., `petl/version.py`) and doesn’t validate the installed artifact.
### Issue Context
- `petl/__init__.py` imports `petl.version`, while `petl/version.py` is gitignored and generated via setuptools-scm.
- The workflow does not `pip install .` / `pip install -e .` / `pip install dist/*.whl` before running pytest and Sphinx.
### Fix Focus Areas
- .github/workflows/test-changes.yml[196-237]
- .github/workflows/test-changes.yml[276-284]
- petl/__init__.py[1-6]
- setup.py[38-42]
- .gitignore[176-178]
### Suggested fix
After `python -m build`, install the built wheel and run tests/docs against that installed distribution, e.g.:
- `python -m pip install dist/*.whl`
- then run `pytest ...`
For docs, either install the wheel first or switch to `pip install -e .` (editable) so imports don’t depend on whether a generated file was left in the checkout.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
/review -i |
| Cython ; python_version < '3.7' or python_version >= '3.9' | ||
| Cython>=0.29.21,<3.0.0 ; python_version >= '3.7' and python_version < '3.9' # fixes further bcolz build | ||
|
|
||
| numpy<2.0.0 ; python_version <= '3.9' # tables==3.9.2 fails with numpy 2.x | ||
| numpy>2.0.0 ; python_version > '3.9' # tables>=3.10 requires numpy>2.0.0 | ||
|
|
There was a problem hiding this comment.
1. Numpy/pytables marker gap 🐞 Bug ☼ Reliability
requirements-formats.txt forces NumPy >2.0.0 for Python >3.9 but only constrains PyTables to >=3.9.2, which still permits selecting tables==3.9.2 alongside NumPy 2.x despite the file’s own compatibility comments. This can yield an unusable environment (install/import/test failures) in full-scope CI jobs that install requirements-formats.txt.
Agent Prompt
### Issue description
`requirements-formats.txt` currently mandates `numpy>2.0.0` for `python_version > '3.9'` but does not also require a `tables` version that is compatible with NumPy 2.x. The same file comments indicate `tables==3.9.2` fails with NumPy 2.x and that `tables>=3.10` requires NumPy 2.x, so the markers should be aligned to prevent an internally-documented incompatible combination.
### Issue Context
This matters because CI installs `-r requirements-formats.txt` for full-scope jobs, so any resolver/platform combination that selects an older `tables` (still allowed by `tables>=3.9.2`) while NumPy is forced to 2.x can break the job.
### Fix Focus Areas
- requirements-formats.txt[1-6]
- requirements-formats.txt[21-22]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 8156d64 |
This PR has the objective of fixing CI issues and upgrading python depencies.
Changes
Checklist
Use this checklist to ensure the quality of pull requests that include new code and/or make changes to existing code.
Includes unit testsNew functions have docstrings with examples that can be run with doctestNew functions are included in API docsDocstrings include notes for any changes to API or behaviortox/pytestmasterbranch and tested before sending the PR