feat: teach find_python_dependencies to scan pyproject.toml and 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
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.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).95 passed.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).