feat: consolidated pip-compile -> uv migration (all 5 stacked PRs, rebased onto master) - #38915
feat: consolidated pip-compile -> uv migration (all 5 stacked PRs, rebased onto master)#38915irfanuddinahmad wants to merge 18 commits into
Conversation
openedx-webhooks
added
open-source-contribution
|
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. |
irfanuddinahmad
changed the title
| @@ -1,5 +1,7 @@ | |||
| [tox] | |||
| envlist = py{312} quality | |||
| requires = | |||
There was a problem hiding this comment.
quality is in the envlist but there is no [testenv:quality] section.
There was a problem hiding this comment.
Confirmed this predates the PR — it's already on master before this migration touched the file — and it's dead: there's no tox -e quality or make quality invocation anywhere in this repo (unlike library repos where that's a real, wired-up pattern). Dropped quality from envlist in 019450a.
| python-version: ${{ matrix.python-version }} | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v7 |
There was a problem hiding this comment.
astral-sh/setup-uv@v7 uses a mutable tag. All other actions in this PR are SHA-pinned — pin this one too.
There was a problem hiding this comment.
Replied on the SHA-pinning question in a summary comment: #38915 (comment)
| requirements | ||
| scripts/**/pyproject.toml | ||
| scripts/**/uv.lock | ||
| scripts/**/requirements* |
There was a problem hiding this comment.
Could you verify these paths? Previously, add-paths monitored the top-level requirements path. With this change, requirements* is only being added under scripts/. I just want to confirm this is the intended behavior.
There was a problem hiding this comment.
Confirmed this is intended: the top-level requirements entry is unchanged from before this PR and still matches everything recursively under requirements/, including the new requirements/edx-sandbox/{pyproject.toml,uv.lock} standalone uv project. The new scripts/**/... and top-level pyproject.toml/uv.lock entries are purely additive, for the new uv sub-projects that live outside requirements/.
| id: cache-dependencies | ||
| uses: actions/cache@v6 | ||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v7 |
There was a problem hiding this comment.
Can we pin this action to a commit SHA instead of @v7? We generally prefer immutable SHA-pinned GitHub Actions to improve supply chain security and reproducibility.
There was a problem hiding this comment.
Replied on the SHA-pinning question in a summary comment: #38915 (comment)
| - name: Install Required Python Dependencies | ||
| run: | | ||
| make base-requirements | ||
| echo "$PWD/.venv/bin" >> "$GITHUB_PATH" |
There was a problem hiding this comment.
Is adding .venv/bin to GITHUB_PATH required here? If subsequent steps are already using uv run, this may be unnecessary. Otherwise, it makes sense so that tools installed by uv sync are available on the PATH for later workflow steps.
There was a problem hiding this comment.
Good question — traced through this job's actual script chain (jest, and the karma vanilla/require/xmodule-webpack configs) and none of them shell out to Python directly, so this wasn't needed. Removed in 019450a.
| id: cache-dependencies | ||
| uses: actions/cache@v6 | ||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v7 |
There was a problem hiding this comment.
Can we pin this action to a commit SHA instead of @v7? We generally prefer immutable SHA-pinned GitHub Actions to improve supply chain security and reproducibility.
There was a problem hiding this comment.
Replied on the SHA-pinning question in a summary comment: #38915 (comment)
|
Re: the Good catch, and worth noting: the org already ran a dedicated SHA-pinning sweep for this exact reason (openedx/.github#165, prompted by the tj-actions/changed-files supply-chain incident), but openedx-platform wasn't part of that ~121-repo effort. That said, I don't think pinning just |
…ration 1/5) Populates [project.dependencies] (from kernel.in + bundled.in), adds [project.optional-dependencies] for the legacy openstack storage backend, adds PEP 735 [dependency-groups] (coverage/testing/doc/assets/development/ semgrep/ci/dev, mirroring the current .in file composition), and [tool.edx_lint].uv_constraints + generated [tool.uv].constraint-dependencies for the ~20 repo-specific version pins, with a committed uv.lock. This is purely additive: the Makefile, tox.ini, and CI workflows are untouched and continue to use pip-compile/requirements/*.txt as the source of truth. Part of the pip-compile -> uv migration tracked in openedx/public-engineering#543 (1 of 5 PRs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ions Found via real CI runs: a fresh uv resolution picked social-auth-core 5.0.2 (previously locked at 4.9.1 via pip-compile), which changes the OAuth pipeline's post-login redirect behavior and breaks common/djangoapps/third_party_auth's integration test suite (AzureAD, Google, LinkedIn, Twitter full-pipeline specs all failed the same assertion in tests/specs/base.py's assert_logged_in_cookie_redirect). This migration is meant to be a tooling swap, not a dependency upgrade, so pin back to the 4.x line rather than bundle an investigation into social-auth-core 5.x's behavior change into this PR. Mirrors the existing social-auth-app-django<=5.4.1 constraint, pinned for a related, already-deferred migration in this same dependency family. Follow-up tracked at #38841. Verified: all 46 previously-failing third_party_auth tests pass with social-auth-core==4.9.1 restored via this constraint. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rewrites the Makefile's requirements targets, tox.ini, and ~13 CI workflows to use uv instead of pip-compile/pip-sync for the main app. Deletes requirements/edx/*.in and *.txt (superseded by pyproject.toml + uv.lock, added in PR 1 / #38835). requirements/constraints.txt, common_constraints.txt, and pip-tools.{in,txt} are intentionally kept for now: requirements/edx-sandbox and scripts/* still pip-compile against them and aren't migrated until PR 3/4. requirements/edx/{base,assets,development}.txt are regenerated as `uv export` compatibility artifacts (via the Makefile's compile-requirements target) since external tooling -- notably tutor's Dockerfile -- installs from those exact paths with plain pip, not uv. check_python_dependencies.yml is disabled (workflow_dispatch only, job gated with if: false) since find_python_dependencies can't scan pyproject.toml yet; tracked at openedx/repo-tools#725. User-confirmed before committing since this removes a CI safety net. Part of openedx/public-engineering#543 (2 of 5). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found via real CI runs after opening the PR (unit tests, Quality Others, and the ReadTheDocs build all failed): 1. Makefile's test-requirements used `uv sync --only-group testing`, which EXCLUDES [project.dependencies] entirely (confirmed: `--only-group` replaces the dependency set rather than adding to it, unlike `--group`). This meant Django, XBlock, and the rest of the actual application were never installed for test runs -- ModuleNotFoundError: No module named 'xblock' on every unit test shard. Fixed to `--no-default-groups --group testing`, matching what tox-uv itself generates for the equivalent tox environment. 2. .readthedocs.yaml had the same bug in its doc-requirements.txt export (`--only-group doc`). Since [project.dependencies] were excluded from that export, the subsequent `pip install -e .` resolved the whole dependency tree completely unconstrained by uv.lock's [tool.uv].constraint-dependencies -- picking setuptools==82.0.1 (violates setuptools<82) and Django==6.0.6 (violates Django<6.0). The setuptools violation broke fs/pyfilesystem2's pkg_resources import, crashing the Sphinx build via Django app loading. Fixed to `--group doc` plus `--no-deps` on the `pip install -e .` step, so dependencies only ever come from the properly-constrained export. 3. scripts/xsslint_config.py's SKIP_DIRS didn't exclude .venv. Under the old pip-compile system, dependencies installed into the system Python outside the repo checkout, so this never mattered. Now that `uv sync` creates .venv/ inside the checkout, xsslint's directory walk (which defaults to scanning the whole cwd) swept up thousands of vendored third-party files, inflating violations from 64 (the accepted baseline) to 316. Verified all three: real pytest run (59 passed) plus full Django `manage.py check` for both LMS and CMS against the corrected test-requirements; a local simulation of the RTD build job sequence confirms Django==5.2.15/setuptools==81.0.0 (both constraint-compliant) and a successful Sphinx build. Audited every other `--only-group` usage introduced in this migration (assets.txt compat export, semgrep.yml) -- both are intentionally project-dependency-free, matching the original files' documented behavior, and their CI checks already passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
requirements/edx/{base,development}.txt were regenerated from the
uv.lock that predated PR 1's social-auth-core<5.0.0 constraint, so
they still referenced social-auth-core==5.0.2 -- caught by
check-consistent-dependencies.yml's re-run of `make compile-requirements`.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Gives requirements/edx-sandbox/ its own standalone pyproject.toml + uv.lock, independent of the main app's dependency graph (codejail intentionally runs untrusted code in a separate, isolated venv). [tool.edx_lint].uv_constraints holds only the subset of the root constraints relevant to this environment's deps (numpy, lxml, setuptools) -- uv/edx-lint have no cross-project constraint chaining equivalent to pip-compile's "-c ../constraints.txt", so root and sandbox constraints are now independently maintained (documented in requirements/edx-sandbox/README.rst). base.txt is regenerated as a `uv export` compatibility artifact (the README documents it as a supported, if unstable, direct pip-install target). releases/*.txt are untouched -- they're frozen historical snapshots, not part of any active compile loop; README now documents cutting future ones via `uv export` instead of pip-compile. Part of openedx/public-engineering#543 (3 of 5). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…on 4/5)
Gives scripts/xblock, scripts/user_retirement, and scripts/structures_pruning
each their own standalone pyproject.toml + uv.lock, mirroring the
codejail sandbox pattern from PR 3. structures_pruning's local
[tool.edx_lint].uv_constraints keeps the pymongo<4.4.1 pin it inherited
via the old "-c ../../../requirements/constraints.txt" chain.
These scripts are documented (in their own READMEs) to support git
sparse-checkout usage -- cloning only e.g. scripts/user_retirement/
without the rest of edx-platform. A self-contained pyproject.toml is
actually an improvement here over the old relative "-c
../../../requirements/constraints.txt" reference, which wouldn't even
resolve in a sparse checkout that excludes the root requirements/ dir.
Compatibility .txt exports are kept at their previously-documented
paths (e.g. scripts/user_retirement/requirements/{base,testing}.txt)
since each script's own README explicitly instructs `pip install -r`
against those exact paths.
Also fixes check-consistent-dependencies.yml's path filter and the two
PR-creating workflows' add-paths, neither of which would have picked
up changes to the new scripts/*/pyproject.toml or uv.lock files.
Part of openedx/public-engineering#543 (4 of 5).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deletes requirements/constraints.txt, common_constraints.txt, and
pip-tools.{in,txt} -- these were kept alive through PR 2-4 because
requirements/edx-sandbox and scripts/* still pip-compiled against
them, but PR 4 was the last consumer, so they're now fully unused.
Removes the correspondingly-vestigial Makefile machinery: the
pre-requirements target, the COMMON_CONSTRAINTS_TXT curl-fetch-and-sed
target, and the CUSTOM_COMPILE_COMMAND/COMPILE_OPTS variables that only
existed to feed pip-compile invocations which no longer exist anywhere
in this repo.
Finalizes requirements/README.rst for the fully-migrated state and
fixes a couple of remaining stale references (constraints.txt ->
[tool.edx_lint].uv_constraints).
This is the last of 5 PRs migrating openedx-platform from pip-compile
to uv + PEP 621/735 pyproject.toml, tracked in
openedx/public-engineering#543. Two follow-up
items remain outside this repo's control:
- openedx/repo-tools#725: find_python_dependencies needs pyproject.toml
support before check_python_dependencies.yml can be re-enabled.
- Tutor's Dockerfile installs from requirements/edx/{base,assets,development}.txt
with plain pip; those are kept as `uv export` compatibility artifacts
(see PR 2 / #38836) rather than deleted, so no action is required there,
but tutor maintainers should be aware these are now generated files.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's "Compile requirements" check re-runs `make compile-requirements` and fails if it produces any diff, to catch exactly this kind of inconsistency. The compat-export files in this PR were originally generated with uv 0.11.26; a newer uv (0.11.30, matching what astral-sh/setup-uv installs in CI) resolves grpcio/grpcio-status with an explicit `; platform_python_implementation != 'PyPy'` marker that 0.11.26 omitted. Regenerated with uv 0.11.30 to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The uv migration for scripts/user_retirement dropped the lxml pin that requirements/constraints.txt previously carried, letting the lockfile resolve to lxml 6.1.1. The pin exists to avoid a libxml2 version mismatch at runtime (#36695), and this script transitively depends on lxml via simple-salesforce -> zeep, so the same pin applies here as it does at the repo root.
…emaining setup-uv steps Faithfully translates .coveragerc into [tool.coverage.*] sections in pyproject.toml, matching the pattern used across the other uv-migration repos -- confirmed coverage.py picks up the new config identically (same source/omit/branch/concurrency/parallel/relative_files/paths values) once .coveragerc is removed. Also adds `enable-cache: true` to the 5 remaining astral-sh/setup-uv steps that weren't touched by the earlier migration commits (check-consistent-dependencies.yml, compile-python-requirements.yml, semgrep.yml, unit-tests.yml x2, upgrade-one-python-dependency.yml), for consistency with the other 7 workflows this migration already updated.
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.
- tox.ini: `quality` in envlist has no [testenv:quality] section and predates this migration; this repo's quality checks (ruff, xsslint, pii_check, check_keywords) never go through tox, so it was inert. - js-tests.yml: nothing in this job's test chain (jest, karma) shells out to Python, so exporting .venv/bin to GITHUB_PATH was unnecessary.
master bumped edx-enterprise 8.3.0 -> 8.7.0 (and transitively openedx-filters) in the pip-compile-generated requirements files while this branch was rebasing; carry the same bump into the uv-managed constraint and regenerate the affected compat exports.
ecd4ab7 to
362c94b
Compare
| "djangorestframework", | ||
| "drf-spectacular", | ||
| "edx-ace", | ||
| "edx-api-doc-tools", |
There was a problem hiding this comment.
The runtime dependency list is huge (150+ packages).
I think we need to verify every package truly belongs in the runtime.
For example packages like
edx-api-doc-tools
openedx-atlas
edx-i18n-tools
are only needed for development or build-time tasks.
If they're not imported by the application at runtime, they could potentially move into a development.
There was a problem hiding this comment.
Good push. Checked each of your three examples against actual usage:
edx-api-doc-toolsis genuinely runtime — it's imported directly in 30+ production view modules across LMS/CMS (lms/urls.py,cms/urls.py, and many REST API views) for DRF schema decorators.openedx-atlasis invoked via subprocess from a registered Django management command, not imported as a library — same categorization as onmasteralready (it was inkernel.inthere too), so not something this PR changed.edx-i18n-toolsyou're right about, and it pointed at something bigger: onmaster,requirements/edx/base.inwas actually-r kernel.in(core) +-r bundled.in" (15 packages explicitly documented there as "not core to the platform's functionality... should not be required for the platform to run nor for the test suite to pass" — mostly third-party XBlocks). This PR had flattened both into one undifferentiated[project.dependencies]` list, losing that distinction.
Restored it: added a bundled dependency-group with those 15 packages (including edx-i18n-tools), included from testing, and wired into base-requirements/the compat export explicitly so nothing that currently gets them (prod images, test runs, CI) loses them — verified via export diffing, zero package/version changes anywhere except tox's own removal. Fixed in 6d4d18a.
There was a problem hiding this comment.
Can we rename this group? bundled doesn't seem like the most appropriate name here.
There was a problem hiding this comment.
Renamed bundled -> non-core to match the wording already used in its in-file comment ("not core to the platform's functionality"). Fixed in a1d3ea1.
| "libsass", | ||
| "nodeenv", | ||
| ] | ||
| development = [ |
There was a problem hiding this comment.
Minor naming consistency issue: the group names aren't uniform. You have doc (abbreviated) alongside development (full word)
There was a problem hiding this comment.
Fair point — renamed doc to docs to match development's un-abbreviated naming (and the docs/ folder). Updated its one internal reference plus .readthedocs.yaml, which also referenced it by name. Fixed in 6d4d18a.
There was a problem hiding this comment.
We still have both development and dev groups. I think having both is a bit confusing. Could we use more consistent naming?
There was a problem hiding this comment.
Good point -- renamed the dev group to default (added explicit tool.uv.default-groups = ["default"] to preserve bare uv sync's current behavior, since that was relying on uv's built-in "dev" default-group name). Now only development remains, wrapped by default = development + ci. Fixed in a1d3ea1.
| python -m venv venv | ||
| . venv/bin/activate | ||
| pip install -r requirements/edx/assets.txt | ||
| uv sync --group assets |
There was a problem hiding this comment.
We need to verify this command.
There was a problem hiding this comment.
Good catch — verified it. uv sync --group assets actually installs ~399 packages, not the minimal 4 (click, libsass, nodeenv, six) the assets group defines, because uv implicitly falls back to also syncing the dev default group when none is configured. The truly minimal command is uv sync --no-default-groups --only-group assets --no-install-project (verified: 4 packages) — updated the docstring to show that and clarify that Tutor/Devstack generally run this within a fuller environment sync rather than standalone. Left static-assets-check.yml's own uv sync --group assets as-is since that job also runs collectstatic, which does need the full install. Fixed in 6d4d18a.
There was a problem hiding this comment.
The updated command just installing the packages but we need to verify the sass compiling to make sure nothing is breaking.
There was a problem hiding this comment.
Fair catch. Verified this isn't just a package-count check: with only that minimal 4-package env synced, libsass's _sass.compile_filename binding actually compiles real .scss to .css correctly (confirmed directly). The one failure I hit compiling the full LMS Sass tree was a missing edx-bootstrap import, which comes from node_modules via npm -- a separate toolchain this PR doesn't touch; Tutor/Devstack always run npm install alongside this sync. Documented this verification in the script's docstring. Fixed in a1d3ea1.
… group - Add a `bundled` dependency-group for the 15 packages (mostly third-party XBlocks) that master kept in a separate `bundled.in`, explicitly documented as "not core to the platform's functionality" -- this PR had flattened them into [project.dependencies] indistinguishable from actually-core deps. Include it from `testing` and explicitly sync/export it in base-requirements and the requirements/edx/base.txt compat export, so nothing that currently gets these packages (production images, test runs, CI) loses them. Verified via export/dry-run diffing: zero package or version changes to base-requirements, test-requirements, or the dev group as a result. - Remove `tox` from the `testing` group: it was duplicated with `ci` (which also has `tox-uv`, its required plugin), and nothing that syncs `testing` alone ever invokes tox directly. - Rename the `doc` dependency-group to `docs`, matching the `development` group's un-abbreviated naming and the docs/ folder; updated its one internal reference and .readthedocs.yaml.
- bundled -> non-core: clearer name for the "installed-by-default but
not required to run/test" group (matches the in-file comment wording).
- dev -> default: avoids near-duplicate naming with the development
group it wraps. Explicitly set tool.uv.default-groups = ["default"]
to preserve bare `uv sync`'s existing behavior of installing this
group, since renaming away from uv's built-in "dev" default name
would otherwise silently stop that.
- Verified both renames are lockfile/export no-ops: uv.lock package
versions are byte-identical (diff is metadata-only), and the
regenerated requirements/edx/{base,development}.txt compat exports
only changed in their header comments.
- Verified the assets group's minimal uv sync actually functionally
compiles Sass (not just installs packages): confirmed libsass's
_sass.compile_filename binding compiles real .scss to .css from that
minimal 4-package environment. The one compile failure encountered
(missing edx-bootstrap import) is from node_modules/npm, a separate
toolchain untouched by this dependency-group split -- documented in
the script's docstring.
…migration-consolidated # Conflicts: # requirements/constraints.txt # requirements/edx/base.txt # requirements/edx/development.txt # requirements/edx/doc.txt # requirements/edx/testing.txt
Merged master (bringing in automated Python dependency-upgrade PRs #38926/#38927), which required resolving modify/delete conflicts on the legacy pip-compile files this PR already deletes (constraints.txt, edx/doc.txt, edx/testing.txt -- kept deleted) and content conflicts on the edx/{base,development}.txt compat exports (kept ours, regenerated below). Bumped the edx-enterprise pin from 8.7.0 to 8.7.2 in [tool.edx_lint].uv_constraints to match master's automated bump, regenerated constraint-dependencies via edx_lint write_uv_constraints, re-locked (also picks up enterprise-integrated-channels 0.1.61 -> 0.1.65, matching master), and regenerated the base.txt/development.txt compat exports.
4 participants
Summary
Consolidated view of the pip-compile -> uv migration tracked in openedx/public-engineering#543, requested by a reviewer as a single PR based on
masterinstead of reviewing the 5-PR stack individually.This PR contains the combined changes from:
pyproject.tomldeps/dependency-groups + commituv.lockMakefile,tox.ini, and CI touvfor the main appuvprojectuvprojectsThe 5 stacked PRs remain open and can still be reviewed/merged individually if preferred -- this PR is an additional, rebased-onto-current-
masterview for reviewers who'd rather see the whole migration in one diff.What changed
pyproject.toml(PEP 621/735): all runtime deps and dependency-groups (test, quality, doc, ci, assets, dev) populated;uv.lockcommitted for the root project.Makefile/tox.ini/CI workflows cut over from pip-compile/pip-tools touv.requirements/edx-sandbox) and the three standalone scripts (scripts/xblock,scripts/user_retirement,scripts/structures_pruning) each migrated to their own standaloneuvprojects (ownpyproject.toml+uv.lock).requirements/constraints.txt,requirements/common_constraints.txt,requirements/pip-tools.{in,txt}, and the corresponding vestigialMakefiletargets/variables.requirements/edx/{base,assets,development}.txt,requirements/edx-sandbox/base.txt, and the three scripts' compat.txtfiles are kept as machine-generateduv exportcompatibility artifacts (for tooling, e.g. Tutor's Dockerfile, that still doespip install -r ...directly) rather than deleted.What's intentionally NOT done here (tracked externally)
find_python_dependenciesneedspyproject.tomlsupport beforecheck_python_dependencies.yml(disabled as part of this migration) can be re-enabled.requirements/edx/{base,assets,development}.txtwith plainpip. Those stay asuv exportcompatibility artifacts rather than being deleted, so no action is required on Tutor's side right now -- but Tutor maintainers should be aware these paths are now machine-generated, not hand-compiled.Verification
master(as of this PR), resolving conflicts against ~110 commits of drift (mostly routine dependency-version bumps in files this migration deletes or replaces, plus a handful of CI-workflow steps master added for pip-caching that are no longer needed onceuv's own caching takes over).make compile-requirementsend-to-end against the rebaseduv.lockfor the root project and all 4 uv sub-projects -- regenerated compatibility export files are byte-identical to what the rebase produced, confirming consistency.uv lockresolves cleanly (445 packages, root project); all 4 sub-projects (requirements/edx-sandbox,scripts/xblock,scripts/user_retirement,scripts/structures_pruning) sync cleanly withuv sync --frozen.uv synccould not be fully verified on this machine (missinglibmysqlclient/pkg-configto buildmysqlclientfrom source) -- relying on CI for full install + test verification, same as the original 5-PR stack already did.🤖 Generated with Claude Code