Repository navigation
Modernize Python tooling (pyproject.toml, uv, src layout) - #730
salman2013 wants to merge 6 commits into
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the pull request, @salman2013! 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #730 +/- ##
==========================================
+ Coverage 73.91% 73.97% +0.06%
==========================================
Files 24 24
Lines 1445 1445
==========================================
+ Hits 1068 1069 +1
+ Misses 377 376 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
irfanuddinahmad
left a comment
There was a problem hiding this comment.
This needs a companion PR to openedx/.github's .github/workflows/repo-health-job.yml, merged together with this one. That workflow still installs edx-repo-health/requirements/pip.txt and pip-tools.txt (deleted here) and runs edx-repo-health/scripts/*.sh (moved to src/scripts/ here), and wg-maintenance's daily cron calls it against master, so the nightly run fails as soon as this merges.
| dependencies = [ | ||
| "dockerfile", | ||
| "github-py @ git+https://github.com/ShineyDev/github.py.git@8e3514010eb35a91e012f2935f48138b3c35cef6", | ||
| "gspread", |
There was a problem hiding this comment.
Please keep gspread<5.12.0 here. The old setup.py published it, and the scripts' plain pip install -e edx-repo-health doesn't see constraint-dependencies, so the scheduled job would pick up gspread 6.x, which constraints.txt notes breaks it. The pytest<8.1.0 pin the scripts got from requirements/base.txt is lost too, so the scripts should install from uv.lock (can go in the same openedx/.github companion PR).
| "*" = ["tests*", "*.tests*"] | ||
|
|
||
| [tool.coverage.run] | ||
| branch = true |
There was a problem hiding this comment.
Please drop branch = true. Master had no coverage config, and enabling branch coverage is what drops codecov/project by 2.43% and fails the check.
…nto modernize-python-tooling Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Keep the gspread<5.12.0 pin in the published dependencies so a plain pip install -e (used by the scheduled scripts) doesn't pull gspread 6.x. Drop branch coverage to match master's coverage config. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tooling # Conflicts: # requirements/base.txt # requirements/ci.txt # requirements/dev.txt # requirements/doc.txt # requirements/quality.txt # requirements/test.txt
irfanuddinahmad
left a comment
There was a problem hiding this comment.
The companion PR to openedx/.github's .github/workflows/repo-health-job.yml is still needed before this merges. It still installs requirements/pip.txt/pip-tools.txt and runs edx-repo-health/scripts/*.sh, so the nightly job will fail once this lands.
| # Install checks and dashboarding script, this should also install pytest-repo-health | ||
| # Install all locked deps (including github-py which is a git+ URL excluded from install_requires) | ||
| pip install -q -r edx-repo-health/requirements/base.txt | ||
| pip install -q -e edx-repo-health |
There was a problem hiding this comment.
Please replace the removed requirements/base.txt install with a locked one, e.g. uv export --frozen --no-emit-project --project edx-repo-health -o reqs.txt && pip install -q -r reqs.txt before this line. Without it, pytest<8.1.0 and cachetools==5.5.2 aren't applied, and the job installs versions the constraints say break it. The companion workflow PR will need to install uv.
| # Install checks and dashboarding script, this should also install pytest-repo-health | ||
| # Install all locked deps (including github-py which is a git+ URL excluded from install_requires) | ||
| pip install -q -r edx-repo-health/requirements/base.txt | ||
| pip install -q -e edx-repo-health |
There was a problem hiding this comment.
Same as in repo-health-script.sh: please install from the lock here too, so pytest<8.1.0 and cachetools==5.5.2 are applied.
| @@ -0,0 +1,2621 @@ | |||
| version = 1 | |||
There was a problem hiding this comment.
Please re-seed uv.lock from master's current requirements/*.txt. Master has had dependency upgrades since the lock was made, so 24 packages are older here than on master (e.g. Django 5.2.19 → 5.2.18, diff-cover 11.0.1 → 10.6.0, soupsieve 4.0.1 → 3.0.2), and merging would downgrade them.
Summary
Migrates this repo from pip-tools/setup.py to modern Python tooling, following the openedx Python Modernization Initiative.
pyproject.toml: PEP 621 project metadata replacessetup.py. License converted to SPDX (Apache-2.0).__version__now resolved viaimportlib.metadatainstead of a hardcoded string. Package discovery now correctly includes all three top-level packages (repo_health,repo_health_dashboard,scripts) and thegithub-pygit dependency, both of which the oldsetup.pysilently excluded frominstall_requiresdue to a bug in itsload_requirementshelper.uv:requirements/*.in/*.txtreplaced by[dependency-groups]+uv.lock.tox.ini,Makefile, and CI (.github/workflows/ci.yml) updated to useuv/tox-uv. Two Jenkins-invoked automation scripts (scripts/dependencies-health-script.sh,scripts/repo-health-script.sh) updated since they referenced the now-deletedrequirements/base.txt.repo_health/,repo_health_dashboard/, andscripts/moved undersrc/. Updated all path references (pyproject.toml,tox.ini,MANIFEST.in,docs/conf.py, doc links, and the Jenkins scripts'--repo-health-path/--configurationarguments, which point at a cloned copy of this repo).Not included: python-semantic-release / PyPI publishing automation (Phase 3), since this repo has never had a PyPI release or a git tag — introducing automatic public releases for a never-published package is a separate decision or a repo maintainer to make, not something this migration should force.
Test plan
make test— 125 tests passmake quality— pylint 10.00/10, isort clean (under this repo's declared Python 3.12)uv lock --check— lockfile in sync withpyproject.tomlpython -m build --wheel— wheel builds cleanly; verifiedrepo_health_dashboard/*.yamlconfig files land in the wheelrun_checks --collect-only— confirmsscripts/run_checks.py's checks-directory discovery still works after thesrc/move.github/workflows/upgrade-python-requirements.yml(callsopenedx/.github/.github/workflows/upgrade-python-requirements.yml@master) is still compatible with the newmake upgrade(uv-based) flow — not verified in this PR