feat: teach find_python_dependencies to scan uv.lock - #735
Conversation
|
Thanks for the pull request, @irfanuddinahmad! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx/repo-tools#725 (find_python_dependencies needs to scan pyproject.toml/uv.lock) now has a proposed fix at openedx/repo-tools#735 -- linking it from the disabled-check comment so re-enabling this workflow is easy to track once it merges/releases.
openedx/repo-tools#725 (find_python_dependencies needs to scan pyproject.toml/uv.lock) now has a proposed fix at openedx/repo-tools#735 -- linking it from the disabled-check comment so re-enabling this workflow is easy to track once it merges/releases.
|
@openedx/axim-engineering hi there! Would someone be able to take a look at this? |
feanil
left a comment
There was a problem hiding this comment.
@irfanuddinahmad this looks good, I don't think it's valuable to parse the pyproject dependencies and also I think it may be a bit misleading. Let's drop that and bump the version and then I think this is good to merge and release.
| yield from _dependency_group_names(all_groups.get(included, []), all_groups, seen) | ||
|
|
||
|
|
||
| def _names_from_pyproject_toml(data): |
There was a problem hiding this comment.
I'm not sure how valuable this is to have. These aren't the resolved full dependencies and so if someone ran the tool against this, they might be mislead into thinking that this tool resolves all the sub-dependencies if we pass the pyproject file instead of a lock file of some sort.
At the very least, we should document that we don't resolve dependencies via the find_python_dependencies tool. But I think it might be better to only support compiled file types like the uv.lock and *.txt files. That way the intent is clear and since we're moving everything to uv.lock, we don't need this at the moment anyway so let's not add more code than we need.
There was a problem hiding this comment.
Done — dropped _names_from_pyproject_toml/_dependency_group_names and the pyproject.toml branch in iter_requirement_names() entirely, along with the now-unused packaging dependency (its only import was in the removed code) and the two pyproject.toml-specific tests. uv.lock/requirements.txt support is unchanged. Bumped __version__ to 4.1.0. Full suite still passes (95 tests).
Per review (feanil, PR openedx#735): scanning pyproject.toml's [project.dependencies]/[dependency-groups] only surfaces direct dependencies, not the resolved transitive closure -- unlike uv.lock or a pip-compile'd requirements.txt, both of which represent a fully resolved dependency graph. Supporting pyproject.toml directly would be misleading (callers could assume this tool resolves dependencies when it doesn't) and isn't needed anyway, since repos migrating off pip-compile land on uv.lock as their fully-resolved file. Removed _names_from_pyproject_toml/_dependency_group_names and the pyproject.toml branch in iter_requirement_names(), the now-unused `packaging` dependency (only import was in the removed code) and its two uv.lock entries, and the two tests exercising pyproject.toml support. uv.lock and requirements.txt support is unchanged. Bumped __version__ to 4.1.0 (new capability: uv.lock support) per review request, ready to merge and release.
|
Pushed — dropped pyproject.toml scanning entirely (kept uv.lock + the existing requirements.txt fallback), removed the now-unused |
find_python_dependencies previously only understood pip-compile style requirements.txt files (via requirements-parser). Repos migrating from pip-compile to pyproject.toml + uv (openedx/public-engineering#506) have no flat requirements file left for it to scan, which forced openedx-platform to disable its Check Python Dependencies workflow entirely (openedx/openedx-platform#38915, tracked in openedx#725). iter_requirement_names() now detects the input file by name: - uv.lock: reads the fully-resolved [[package]] list, matching the same direct+transitive closure a pip-compile'd requirements.txt used to represent. - pyproject.toml: reads [project.dependencies], [project.optional-dependencies], and [dependency-groups], resolving {include-group = "..."} references. - anything else: falls back to the existing requirements.txt parsing, unchanged. Verified against openedx-platform's actual pyproject.toml (202 names) and uv.lock (445 names). Closes openedx#725
Per review (feanil, PR openedx#735): scanning pyproject.toml's [project.dependencies]/[dependency-groups] only surfaces direct dependencies, not the resolved transitive closure -- unlike uv.lock or a pip-compile'd requirements.txt, both of which represent a fully resolved dependency graph. Supporting pyproject.toml directly would be misleading (callers could assume this tool resolves dependencies when it doesn't) and isn't needed anyway, since repos migrating off pip-compile land on uv.lock as their fully-resolved file. Removed _names_from_pyproject_toml/_dependency_group_names and the pyproject.toml branch in iter_requirement_names(), the now-unused `packaging` dependency (only import was in the removed code) and its two uv.lock entries, and the two tests exercising pyproject.toml support. uv.lock and requirements.txt support is unchanged. Bumped __version__ to 4.1.0 (new capability: uv.lock support) per review request, ready to merge and release.
b93e6b3 to
2609cb9
Compare
|
Two small docs fixes before merge: PR title still says "scan pyproject.toml and uv.lock" — pyproject.toml was dropped in the second commit. Suggest updating to PR description claims two Before this merges, it would be good to see a quick smoke test across both supported formats to confirm nothing regresses on real-world repos. Install from this branch: pip install "git+https://github.com/irfanuddinahmad/repo-tools.git@irfanuddinahmad/find-python-dependencies-pyproject-uv#egg=edx-repo-tools[find_dependencies]"Test against a find_python_dependencies --req-file path/to/openedx-platform/uv.lockTest against a pip-compile find_python_dependencies \
--req-file path/to/repo/requirements/production.txt \
--req-file path/to/repo/requirements/testing.txtCould you share the terminal output for both? Specifically: how many packages were scanned and whether any second-party packages were flagged. I have merging rights — happy to merge this once the above points are addressed.
|
PR description claimed this test already existed; farhan caught the gap.
|
Addressed both items and ran the requested smoke tests, installing from this branch (
Smoke test 1 — 445 packages scanned (matches the count already in the PR description), 2 second-party packages flagged, exit code 1. Smoke test 2 — pip-compile 355 unique packages scanned across both files, same 2 packages flagged, exit code 1. Confirms the pre-existing Both formats behave as expected on real-world repos — ready for another look. |
….lock openedx/repo-tools#735 (merged today, 13:53, 43 minutes before feanil's review) taught find_python_dependencies to scan uv.lock directly. Revert the workaround this workflow was disabled with: restore the pull_request/ merge_group triggers, drop if: false, and run it via `uvx --from edx-repo-tools[find_dependencies]` against uv.lock instead of pip-installing the tool and pointing it at the now-deleted requirements/edx/*.txt files. Verified locally against this repo's real uv.lock: scans all packages, correctly identifies the two pre-approved second-party dependencies (edx-sga, xblock-poll), and exits 0 once they're passed via --ignore.
What
find_python_dependenciespreviously only knew how to scan pip-compile stylerequirements.txtfiles (viarequirements-parser) for dependencies whose PyPI-registered homepage belongs to a "second party" org (edx-unsupported, mitodl, overhangio, open-craft, eduNEXT, raccoongang) rather thanopenedxitself, so they can be flagged for review/migration.Repos migrating from pip-compile to
pyproject.toml+uvper the org-wide Python packaging modernization (openedx/public-engineering#506) have no flat requirements file left for it to scan. This forced openedx-platform to disable itsCheck Python Dependenciesworkflow entirely as part of its own migration (openedx/openedx-platform#38915), rather than have it silently no-op afterrequirements/edx/{base,testing}.txtwere removed.This closes #725.
How
iter_requirement_names()now branches on the input file name:uv.lock: reads the fully-resolved[[package]]list — this matches the same direct+transitive dependency closure a pip-compile'drequirements.txtused to represent, so it's the most faithful drop-in replacement for the old--req-file requirements/edx/base.txtusage.requirements.txtparsing, unchanged.pyproject.tomlscanning was deliberately left out (see review discussion below): its[project.dependencies]/[dependency-groups]only list direct dependencies, not the resolved transitive closure, so scanning it directly would be misleading about what this tool actually checks.main()also now dedupes names-per-file before hitting the PyPI API.Bumped
__version__4.0.0 → 4.1.0 for this new capability.Side effect worth calling out: scanning
uv.lockbroadens what gets checkeduv.lockis a single unified lockfile covering every dependency-group at once (base,testing,doc,quality,assets,coverage,semgrep, etc.) — there's no way to scan "just base+testing" from it the way the old per-file.txtlayout allowed._names_from_uv_lockyields every package in the lock with no group filtering.Concretely, for openedx-platform: the pre-migration check only ever scanned
requirements/edx/base.txt+requirements/edx/testing.txt(confirmed by reading that repo's owncheck_python_dependencies.yml, both before and after its migration commit). Once it re-enables by pointing--req-fileatuv.lock, the check will also start flagging second-party-org packages living in the previously-invisibledoc/quality/assets/coverage/semgrep/developmentgroups.This is arguably a net positive for supply-chain coverage — a compromised or unvetted dev-tooling package still executes code on CI runners and dev machines, with access to secrets/tokens, so that's a real risk surface too, not just production code. But it is a real behavior change, not a like-for-like swap: if any of those dev-only groups currently has an unvetted second-party dependency, the check will start failing on first re-enable where it never would have before.
Verification
tests/test_find_python_dependencies.pycovers both input formats (requirements.txt,uv.lock) plus twomain()-level tests (one that should flag a second-party dependency, one that should respect--ignore).107 passed(confirmed via the actual CI run log, not just locally).iter_requirement_names()directly against openedx-platform's realuv.lock(445 names) to confirm it doesn't choke on a real-world file.Follow-up
Once this is released, openedx-platform's
check_python_dependencies.ymlcan point--req-fileatuv.lockand be re-enabled (currentlyworkflow_dispatch-only +if: false, per the comment there referencing this issue) — see the scope-broadening note above before doing so.