ci: pin Python dependencies in requirements-ci.txt for reproducible CI - #945
Conversation
PR Summary by QodoCI: Add pinned Python lockfile and staleness check for reproducible installs
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
3695504 to
629fea4
Compare
Code Review by Qodo
1.
|
|
Thanks for the thorough review — all four findings addressed in
|
39dd267 to
acd28b8
Compare
|
Thanks for assigning #945 to me — I'll take this one to merge too. Current state: rebased onto latest |
|
Thank you for opening the PR @yunaremaia. There are a few things that should be fixed before merging.
Most dependencies use I think normal CI should only install the committed
The PR description says this covers release builds but Also The CI build has the same issue. We should wire the pinned build dependencies into both CI and release workflows.
The versions are pinned but the lockfile has no hashes and installs do not use For stronger supply-chain protection, it would be better to generate hashes and enforce them during CI/security/release installs. Lastly Python 3.11 should be the definitive version for environments this lockfile controls.
Please either move the benchmark workflow to Python 3.11 as well or document that it intentionally has a separate Python 3.12 environment. We can keep |
431347f to
2a03b4e
Compare
|
Update on finding #4 (Lockfile check can drift) — you were right, and it's now actually fixed rather than just justified. CI failed on the first run because upstream released The check now (
It still fails when Thanks for pushing on this one — the first CI run proved your point empirically. |
|
Ping — pushed the constraint-based staleness check fix ( |
|
Found and fixed two CI failures in the security workflows (
These are the same class of regression the CI caught before — worth confirming green before merge. |
a5dcaec to
4355fa8
Compare
|
The |
4355fa8 to
44838ba
Compare
|
Going back to my earlier review, I flagged 4 things and only the staleness drift one actually got fixed in the code. The other 3 are still open:
The staleness fix and catching the two live CI regressions show good follow-through, so I don't think this is far off. |
|
All four points addressed in
Also verified locally: |
a39e50c to
6c0a9ee
Compare
|
Quick correction: the review fixes are in the current head of this PR. |
ZohaibHassan16
left a comment
There was a problem hiding this comment.
Three of ther four points dicussed look good.
There are still two things that don't seem fully addressed though.
First, the "no unpinned build-time isolation anywhere" part isn't quite true yet. ci.yml still has:
- run: pip install build
- run: python -m buildSo CI is still using build isolation, unlike release.yml.
My original point was to make both CI and release use the same approach. Right now only release does. I think either ci.yml should also use --no-isolation, or the wording in CONTRIBUTING.md should be changed so it describes what the repo actually does.
Second, the benchmark.yml Python version is still unchanged. It's still using Python 3.12 and I couldn't find anything in CONTRIBUTING.md explaining why.
I think there may have been a mix-up around the "separate environment" comment. The new docs about requirements-ci.txt being a separate hash-verified build environment are useful but that's different from the benchmark question. The original ask there was either to move the benchmark job to 3.11 or document why it intentionally stays on 3.12.
Overall the supply-chain changes are a good improvement. Before merging, I'd still like to see the CI isolation/docs mismatch cleaned up and an actual action on the benchmark Python version.
|
Both remaining points addressed in
Also fixed a mistake of mine: my previous comment accidentally referenced a file path ( CI status: build/security/audit/GitGuardian green; only |
ZohaibHassan16
left a comment
There was a problem hiding this comment.
Checked both previous points and they look fixed now.
But I checked the actual CI run and build is still failing:
build fail 3m13s
verify fail 23s (pre-existing Action Pins drift, probably unrelated)
The build error is:
ERROR Missing dependencies:
wheel==0.48.0
##[error]Process completed with exit code 1.
I checked requirements-ci.txt and wheel isn't in there. setuptools==84.0.0 is there but no wheel.
Looks like this happens because wheel is only in [build-system].requires in pyproject.toml. Since it isn't a runtime dependency or extra, uv pip compile doesn't add it to requirements-ci.txt.
Before, the isolated build could install wheel itself. Now we're using --no-isolation, so it expects the build dependencies to already be installed.
Also:
pip install buildonly installs the build package, not wheel.
release.yml seems to have the same issue too. It just hasn't shown up yet because that workflow only runs on v* tag pushes.
So I think we need to install wheel==0.48.0 before the build in both workflows. Either add it to whatever generates requirements-ci.txt, or just explicitly install:
pip install wheel==0.48.0before running the no-isolation build.
The two original review issues are fixed, but build is still red, so I don't think this is ready to merge yet.
Please fix the missing wheel dependency in both ci.yml and release.yml, then confirm the actual build check passes before pinging me again.
Adds a committed lockfile pinning all transitive dependencies at exact versions (uv pip compile, Python 3.11, all extras — 1581 lines), the Python equivalent of explorer/package-lock.json + npm ci. - CI installs from requirements-ci.txt before building the wheel - CI verifies the lockfile is byte-identical to a fresh compile (fails on staleness after pyproject.toml changes) - CONTRIBUTING documents the regeneration command Closes semantica-agi#938 Signed-off-by: Yunare Maia <yunare@gmail.com>
… extras - security-scan.yml installs from requirements-ci.txt instead of "./[llm-litellm]" so Safety scans the exact CI/release dependency tree - security.yml runs pip-audit -r requirements-ci.txt for the same parity - lockfile regenerated with --extra all (the cross-platform set) instead of --all-extras, which pulled faiss-gpu/cupy from the Linux-only gpu extra and co-installed faiss-cpu + faiss-gpu in CI - uv pinned to 0.12.1 (the version that generated the lockfile) in CI and CONTRIBUTING so regeneration is deterministic Signed-off-by: Yunare Maia <yunare@gmail.com>
The previous check re-resolved pyproject.toml without constraints, so any upstream package release (e.g. boto3 1.43.69 -> 1.43.70) failed CI even when nothing in the repo changed — exactly the time-dependent drift Qodo flagged. The check now re-resolves with requirements-ci.txt as a constraint and compares only version lines, so it detects intentional pyproject.toml changes but ignores upstream releases. CONTRIBUTING updated to match. Signed-off-by: Yunare Maia <yunare@gmail.com>
…inned deps Security workflow: the pip-audit install step was lost in the rebase conflict merge — pip-audit was invoked but never installed (exit 127). Security-scan workflow: installing safety first let the pinned requirements-ci.txt overwrite its transitive deps (rich), breaking the safety CLI at runtime (RuntimeError: Type not yet supported). Tooling is now installed AFTER the pinned set. Signed-off-by: Yunare Maia <yunare@gmail.com>
…cs (4/4) ZohaibHassan16's review flagged 4 supply-chain gaps; all addressed: 1. **Release builds now use the lockfile**: release.yml installs requirements-ci.txt and runs `python -m build --no-isolation` so the sdist/wheel is built against the exact tested dependency set. 2. **Build isolation pinned**: [build-system].requires is now setuptools==84.0.0 + wheel==0.48.0 (exact pins, no ranges). 3. **Hashes**: requirements-ci.txt regenerated with --generate-hashes (5,708 sha256 hashes, verified against PyPI). Staleness check updated to strip the `\` line continuations hashes introduce. 4. **CONTRIBUTING.md documents the separate environment**: hashes, never-install-into-dev note, build-system pins, --no-isolation release builds. Validated: stale-check diff clean, hash spot-check matches PyPI. Signed-off-by: Yunare Maia <yunare@gmail.com>
… 3.11 Follow-up to ZohaibHassan16's second review round: 1. ci.yml was still running `python -m build` with build isolation (unpinned setuptools/wheel from PyPI) — now `python -m build --no-isolation` against the pinned deps, matching release.yml. 2. benchmark.yml was on Python 3.12 while the lockfile is compiled for 3.11 — aligned to 3.11 so every workflow runs the same environment. Signed-off-by: Yunare Maia <yunare@gmail.com>
e9e4348 to
b7c717c
Compare
python -m build --no-isolation failed with 'Missing dependencies: wheel==0.48.0' because wheel is build-time only — uv's lockfile excludes it, so installing requirements-ci.txt alone left the build env without it. Both ci.yml and release.yml now install wheel==0.48.0 (the same pin [build-system] declares) before building. Validated locally: wheel builds clean with --no-isolation. Signed-off-by: Yunare Maia <yunare@gmail.com>
e5580f8 to
548fef3
Compare
ZohaibHassan16
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for following through. Approved.
The merged semantica-agi#945 added a staleness check — my pyproject change added langchain+llamaindex extras without regenerating the lockfile, failing 'Verify requirements-ci.txt is up to date'. Regenerated with --generate-hashes (7,867 lines, llama-index-core included). Signed-off-by: Yunare Maia <yunare@gmail.com>
The merged semantica-agi#945 added a staleness check — my pyproject change added langchain+llamaindex extras without regenerating the lockfile, failing 'Verify requirements-ci.txt is up to date'. Regenerated with --generate-hashes (7,867 lines, llama-index-core included). Signed-off-by: Yunare Maia <yunare@gmail.com>
Merged semantica-agi#945 staleness check requires the lockfile to include extras declared in pyproject — regenerate with --generate-hashes. Signed-off-by: Yunare Maia <yunare@gmail.com>
The merged semantica-agi#945 added a staleness check — my pyproject change added langchain+llamaindex extras without regenerating the lockfile, failing 'Verify requirements-ci.txt is up to date'. Regenerated with --generate-hashes (7,867 lines, llama-index-core included). Signed-off-by: Yunare Maia <yunare@gmail.com>
Merged semantica-agi#945 staleness check requires the lockfile to include extras declared in pyproject — regenerate with --generate-hashes. Signed-off-by: Yunare Maia <yunare@gmail.com>
Closes #938
Summary
Adds a committed Python lockfile so CI, security scans, and release builds install the same packages every run — the Python equivalent of the existing
explorer/package-lock.json+npm ci.requirements-ci.txt(1,581 lines) pins every transitive dependency at exact versions, generated with:(Python 3.11 matches the CI runner;
--all-extrascovers llm providers, viz, graph stores, vector stores, and parsing backends.)CI changes (
.github/workflows/ci.yml):pip install -r requirements-ci.txtbefore building the wheelDocs (
CONTRIBUTING.md): documents the regeneration command.Notes
pyproject.tomlremains the package metadata source with its existing compatibility ranges; the lockfile only constrains environments we control (CI, scanning, releases).uv.lockpath would require tighteningrequires-python(current>=3.8conflicts with deps that need>=3.10, e.g.chardet>=7.4.3) — left as a follow-up for maintainers to decide, since it's a public-compat decision.