Skip to content

fix: --no-deps on the edx-platform editable install so manifest overrides survive - #214

Merged
blarghmatey merged 2 commits into
mainfrom
fix/overrides-after-editable-install
Sep 3, 2026
Merged

blarghmatey merged 2 commits into
mainfrom
fix/overrides-after-editable-install

Conversation

@blarghmatey

Copy link
Copy Markdown
Member

What are the relevant tickets?

N/A

Description (What does it do?)

Adds --no-deps to the three uv pip install -e . calls on edx-platform in src/lehrer/core/platform.py.

A deployment override in build_manifest.yaml could be silently discarded, producing an image that did not contain the code the manifest asked for.

The build order is: install edx-platform's dependency set → apply the cell's overrides: → uv pip install -e . on edx-platform. That last step re-resolves edx-platform's own declared dependencies from the index, so any override naming a package edx-platform declares gets replaced by the indexed pin, and its dist-info loses direct_url.json.

Why this was hard to see: it splits by release line.

  • edx-platform master migrated its full dependency list into pyproject.toml and declares "openedx-forum" there → the git+https://github.com/mitodl/forum.git@tmacey/fix-typesense-search-params#egg=openedx-forum override was reverted to indexed openedx-forum==0.4.3 on all three master cells.
  • release/verawood predates that migration: its pyproject.toml declares dependencies = ["setuptools"] and nothing else, so the editable install had nothing to re-resolve and the same manifest line survived as 0.4.5 with direct_url.json intact. (Its requirements/edx/base.txt does pin openedx-forum==0.4.2, but that file is not what uv pip install -e . reads.)

Identical manifest syntax, images rebuilt after the override merged, opposite results.

--no-deps is safe here: _edx_base_deps_script() has already installed the full dependency set (requirements/edx/base.txt + assets.txt, or the uv sync groups on master), and the overrides step runs after it. This install has nothing left to resolve and only needs to register the package so lms/cms and their console entry points work.

Applied at all three sites. The test-harness one (platform.py in the test container builder) matters too: without it the suite runs against packages the shipped image does not contain, which is the class of gap that let the Typesense search bugs ship in the first place.

How can this be tested?

uv run pytest tests — 306 passed. pre-commit run clean (ruff, mypy, core-boundary check).

Neither covers the change itself: no test in this suite asserts on the dagger container chain, and adding that would mean a new mocking harness. Flagging that as a known gap rather than papering over it. If you would like a guard, the cheapest one is a source-level assertion that every editable install of edx-platform carries --no-deps, though that is arguably brittle. Happy to add it if you want it.

What the change was verified against is the deployed CI images, read directly from the running pods:

mitxonline.CI  (master)   openedx_forum-0.4.3.dist-info  direct_url.json: absent
mitx.CI        (master)   openedx_forum-0.4.3.dist-info  direct_url.json: absent
mitx-staging.CI(master)   openedx_forum-0.4.3.dist-info  direct_url.json: absent
xpro.CI        (verawood) openedx_forum-0.4.5.dist-info  direct_url.json: PRESENT

direct_url.json is written by pip/uv for any direct-URL (git/VCS/local) install and never for a plain index install, so it is a definitive yes/no on whether an override took. To confirm the fix after a rebuild, re-run against a master cell and expect 0.4.5 + direct_url.json present:

kubectl -n <ns> exec deploy/lms-edxapp-app -c lms-edxapp-app -- python -c "
import glob, os
d = glob.glob('/openedx/venv/lib/python*/site-packages/openedx_forum-*.dist-info')
print([os.path.basename(x) for x in d],
      'from_git:', any(os.path.exists(os.path.join(x, 'direct_url.json')) for x in d))"

Additional Context

  • This has a live consequence. Enable Typesense forum search for master/verawood CI+QA environments ol-infrastructure#5718 moved forum search onto Typesense in seven CI/QA environments on the assumption that every master and verawood cell carried the openedx/forum#289 fix. Three master CI environments are currently pointed at the unfixed 0.4.3 backend. Confirmed by the query each one emits: broken cells send per_page=1000 (over Typesense's 250 cap, HTTP 422 once a collection exists), the fixed xpro cell sends per_page=250&page=1.
  • Practical impact is contained: those CI environments served zero real forum searches over the preceding 30 days. The cost is that verification is untrustworthy until the images are rebuilt, since a tester cannot tell a config problem from an image problem.
  • Worth a look from someone who knows the uv sync arm on master: _edx_base_deps_script() uses uv sync --inexact with groups there, which may already install the project itself, making the editable install partly redundant on that path. --no-deps is correct either way, but the redundancy might be worth removing separately.

A deployment override in build_manifest.yaml could be silently discarded,
producing an image that did not contain the code the manifest asked for.

The build installs edx-platform's dependency set, applies the cell's
overrides, and then runs `uv pip install -e .` on edx-platform. That last
step re-resolves edx-platform's own declared dependencies from the index, so
any override naming a package edx-platform declares is replaced by the
indexed pin and its dist-info loses direct_url.json.

This split by release line, which is what made it hard to see. master's
pyproject.toml declares "openedx-forum", so the
`git+https://github.com/mitodl/forum.git@...#egg=openedx-forum` override was
reverted to the indexed openedx-forum==0.4.3 on all three master cells.
release/verawood does not declare it, so the same manifest line survived
there as 0.4.5 with direct_url.json intact. Identical manifest syntax, images
rebuilt after the override merged, opposite results.

Observed in the deployed CI images: mitxonline, mitx and mitx-staging all
carried 0.4.3 with no direct_url.json, while xpro carried 0.4.5 installed
from git. ol-infrastructure had already moved forum search onto Typesense in
those environments on the assumption the fix was present, so three of them
were running the unfixed backend.

--no-deps is safe here: _edx_base_deps_script() has already installed the
full dependency set (requirements/edx/base.txt + assets.txt, or the uv sync
groups on master) and the overrides step ran after it, so this install has
nothing to resolve and only needs to register the package.

Applied at all three editable-install sites. The test-harness one matters
too: without it the suite runs against packages the shipped image does not
have, which is the class of gap that let the Typesense search bugs ship.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012c3Ua8fPhRYnEdQE8snFNW
Copilot AI balanced review requested due to automatic review settings September 2, 2026 19:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The subsequent test dependency sync can still replace deployment overrides with lockfile versions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Prevents editable edx-platform installs from overwriting deployment dependency overrides.

Changes:

  • Adds --no-deps to three editable-install paths.
  • Documents why dependency resolution must remain disabled.
File summaries
File Description
src/lehrer/core/platform.py Preserves manifest overrides during build, verification, and test setup.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/lehrer/core/platform.py Outdated
Comment thread src/lehrer/core/platform.py Outdated
Comment thread src/lehrer/core/platform.py Outdated
…le install

Addresses review feedback on #214.

The test path was still losing the override. --no-deps stops the editable
install from re-resolving, but _edx_testing_deps_script() runs straight after
and reconciles any package the checkout itself pins back to the checkout's
version. --inexact does not prevent that; it only stops extraneous packages
being pruned. Confirmed against uv directly:

    uv sync --locked --active --no-install-project --inexact \
            --no-default-groups --group testing
    - packaging==23.2
    + packaging==24.0

Both arms are affected, so this is not master-only: master's uv.lock pins
openedx-forum==0.4.3, and release/verawood's requirements/edx/testing.txt
pins openedx-forum==0.4.2. Re-applying the cell's overrides after the testing
step keeps them the final dependency layer and the test environment faithful
to the shipped image. Passing only the -r file is deliberate -- the
source-built lxml/xmlsec already satisfy the versions pinned there, so uv
leaves them alone rather than swapping in wheels.

--only-group testing would fix the sync arm alone (verified: it installs the
group and leaves an off-lock package untouched) but does nothing for the
legacy arm, so the re-apply covers both instead.

Centralizes the editable install in _edx_platform_editable_install() so one
call site cannot regress independently, with regression tests asserting the
flag -- no Dagger mock needed, following the existing _edx_*_script() helper
tests. Its docstring also records why the install is editable at all: the
dist-info carries 133 entry points including every core XBlock, and the build
writes the Django settings modules into the tree after this runs.

Also corrects a cross-reference to a function name that does not exist
(collect_artifacts -> the flag now lives in the shared helper).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012c3Ua8fPhRYnEdQE8snFNW
@blarghmatey
blarghmatey merged commit db23d49 into main Sep 3, 2026
12 checks passed
@blarghmatey
blarghmatey deleted the fix/overrides-after-editable-install branch September 3, 2026 13:54
blarghmatey added a commit that referenced this pull request Sep 3, 2026
…des (#215)

* fix: set UV_NO_SYNC so uv run can't silently revert deployment overrides

Even after #214's --no-deps and testing-sync fixes, a rebuilt master image
still shipped the pre-override openedx-forum (0.4.3, no direct_url.json)
rather than the git-installed 0.4.5 the manifest override asked for --
verified by pulling the pushed image directly and inspecting its dist-info.

Root cause is a third occurrence of the same bug class, from a source outside
this codebase entirely: master edx-platform's own package.json runs
`compile-sass` as `uv run --active python scripts/compile_sass.py`. `uv run`
resyncs the active venv against pyproject.toml/uv.lock before running
anything unless told not to, and master's pyproject.toml declares
openedx-forum -- so build_static_assets()'s `npm run compile-sass` reverts
the override AGAIN, after both the overrides step and the --no-deps editable
install had already left the correct version in place.

release/verawood's compile-sass script is a plain `scripts/compile_sass.py`,
no `uv run`, which is why that release line never showed this and why it
wasn't caught by #214's review.

Reproduced directly against uv: `uv run --active python -c ...` on a venv
holding an off-lock package reverts it; `UV_NO_SYNC=1 uv run --active ...`
does not. `UV_NO_SYNC` has no effect on `uv sync`/`uv pip install`, so it is
safe to set unconditionally without touching the explicit `_edx_base_deps_script()`
sync or any other uv subcommand this pipeline runs.

Set in two places because env vars don't survive the multi-stage copy:
install_deps() protects check_deployment() and _prepare_test_run(), which
chain directly off it; build_platform()'s clean-base reconstruction (only
directories carry across from the throwaway `deps` container, not env vars)
needs its own copy to protect collected()/build_static_assets()/docker_image().

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012c3Ua8fPhRYnEdQE8snFNW

* fix: deduplicate a repeated clause in the UV_NO_SYNC comment

Addresses review feedback on #215. The clause "of a package master's
pyproject.toml declares" was left in twice from an earlier edit that renamed
mitxonline to a generic "a package" to satisfy the core-boundary check,
without removing the sentence it was duplicating.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012c3Ua8fPhRYnEdQE8snFNW

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants