Make pip-tools work again - #89
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
The pip pin in the workflow likely won’t affect the pip/pip-tools install performed inside tox, so the reported failure may still reproduce.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR attempts to restore CI stability by constraining the pip version in GitHub Actions, motivated by a pip-tools import failure observed when running tox environments on Python 3.12.
Changes:
- Pin
pipto<26in the GitHub Actions “Install dependencies” step.
File summaries
| File | Description |
|---|---|
| .github/workflows/tests.yml | Pins the runner’s pip version to avoid breakage from newer pip releases. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
a404620 to
935a5f8
Compare
935a5f8 to
1f89a8e
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The updated tox.ini command has broken shell quoting ('pip<26' inside an outer single-quoted string), which will prevent tox environments from being created.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
1f89a8e to
00e594c
Compare
There was a problem hiding this comment.
🟢 Ready to approve
The change is minimal and targeted to tox bootstrapping, with only a small maintainability note to consider about documenting the new pip pin.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
tox.ini:34
- The new
pip<26constraint is a behavioral workaround but it isn’t documented in the config itself. Adding a short note (and ideally an upstream reference) will help future maintainers know when/why this pin exists and when it can be removed/updated.
sh -c 'python -I -m pip install -U "pip<26" pip-tools && python -m piptools sync requirements/py{py_dot_ver}-production.txt requirements/py{py_dot_ver}-test.txt && python -I -m pip install "$@"' -
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The change is a minimal, localized mitigation in tox configuration that directly addresses the reported runtime failure without altering project runtime code.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The change is narrowly scoped to tox environment bootstrapping and directly addresses the documented piptools import failure by constraining pip to a compatible range.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
697aae3 to
55224b1
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The tox command now relies on per-Python pinned requirements files that are missing for py312 in the repo, which would make tox -e py312 fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
tox.ini:33
- The new hard pins (
pip<26and especiallypip-tools<7.6) are an implicit long-term constraint that can block future compatibility/security updates. If thepip-toolsupper bound is required, please document the specific incompatibility (e.g., upstream issue/PR) in an adjacent comment; otherwise consider removing thepip-tools<7.6cap and only pinning the brokenpipmajor version.
sh -c 'python -I -m pip install -U "pip<26" "pip-tools<7.6" && python -m piptools sync requirements/py{py_dot_ver}-production.txt requirements/py{py_dot_ver}-test.txt && python -I -m pip install "$@"' -
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
55224b1 to
2b79dda
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
tox.ini references requirements/py{py_dot_ver}-*.txt but the repo only contains requirements/py3.10-*.txt, so the py312 env will still fail regardless of the new pip/pip-tools pins.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
tox.ini:34
- The install_command syncs
requirements/py{py_dot_ver}-production.txt/-test.txt, but the repo currently only containsrequirements/py3.10-*.txt. This makes thepy312tox env fail with a file-not-found error duringpiptools sync(andenvlistincludes py312, and GH Actions maps 3.12 -> py312). Add the missingrequirements/py3.12-*.txtfiles (or adjust tox to use an existing requirements set / drop py312) so py312 can run.
sh -c 'python -I -m pip install -U "pip<26" "pip-tools<7.6" && python -m piptools sync requirements/py{py_dot_ver}-production.txt requirements/py{py_dot_ver}-test.txt && python -I -m pip install "$@"' -
tox.ini:34
- The new
pip<26/pip-tools<7.6pins are a behavioral workaround but aren’t explained in-file; the surrounding comment block only describes the shell hack. Adding a brief note about the underlyingpiptoolsImportError (and when to revisit/remove the pins) will help future maintainers understand why tooling is constrained here.
# ugly hack using sh -c... (YL 2025-03-05 using tox 4.24.1)
allowlist_externals = sh
install_command =
sh -c 'python -I -m pip install -U "pip<26" "pip-tools<7.6" && python -m piptools sync requirements/py{py_dot_ver}-production.txt requirements/py{py_dot_ver}-test.txt && python -I -m pip install "$@"' -
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
@copilot I am sorry, but as far as I can see the repo does contain |
2b79dda to
c31c60e
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The new pinning is inconsistent with the unpinned pip-tools entry in deps, which can reintroduce a broken pip-tools version, and there is also a small typo in the added comment.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
tox.ini:36
- The pip-tools version cap is applied in install_command ("pip-tools<7.6"), but the tox deps list still requests unpinned "pip-tools". If pip-tools gets removed during
piptools sync(it is not pinned in requirements/*.txt), the finalpip install "$@"can reinstall a newer pip-tools and reintroduce the breakage. Pin pip-tools in deps as well to keep the constraint consistent.
install_command =
sh -c 'python -I -m pip install -U "pip<26" "pip-tools<7.6" && python -m piptools sync requirements/py{py_dot_ver}-production.txt requirements/py{py_dot_ver}-test.txt && python -I -m pip install "$@"' -
tox.ini:33
- Typo in the comment: the package is named "pip-tools" (plural), not "pip-tool".
# - pip-tool 7.6 doesn't work for us
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
c31c60e to
4420e37
Compare
4420e37 to
322d756
Compare
It looks like pip-tools fails to run with recent versions of pip:
Traceback (most recent call last):
File "<frozen runpy>", line 198, in _run_module_as_main
File "<frozen runpy>", line 88, in _run_code
File "neurospin_to_bids/.tox/py312/lib/python3.12/site-packages/piptools/__main__.py", line 5, in <module>
from piptools.scripts import compile, sync
File "neurospin_to_bids/.tox/py312/lib/python3.12/site-packages/piptools/scripts/sync.py", line 17, in <module>
from .. import sync
File "neurospin_to_bids/.tox/py312/lib/python3.12/site-packages/piptools/sync.py", line 13, in <module>
from pip._internal.utils.compat import stdlib_pkgs
ImportError: cannot import name 'stdlib_pkgs' from 'pip._internal.utils.compat'
(neurospin_to_bids/.tox/py312/lib/python3.12/site-packages/pip/_internal/utils/compat.py)
322d756 to
b5482d7
Compare
|
@copilot Please have a look again, I think I have addressed all issues. |
There was a problem hiding this comment.
🟢 Ready to approve
The change is a straightforward version pin to restore tooling functionality, with only a minor comment typo to address.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
tox.ini:33
- Typo in the comment: it says "pip-tool" but the project/package name is "pip-tools".
# - pip-tools 7.6 doesn't work for us
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
It looks like pip-tools fails to run with recent versions of pip: