Repository navigation
bug: ship the modules json as a placeholder so uninstall removes it - #704
Conversation
The CLI downloads modules-<version>.json into its package folder. pip did not install that file, so uninstalling left it and the lean folder behind, where Python picks it up as a namespace package that shadows a later editable install. setup.py now writes an empty placeholder and ships it as package data, so pip records the file and removes it on uninstall. lean.models treats an empty file as not downloaded. Closes #703 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
CI note: The README diff adds
This change downloads the same file the same way as master, so it doesn't alter what the README is generated from. Once every CDN edge serves the new modules file, the README check will fail on master and on every branch until |
There was a problem hiding this comment.
Approving. This implements the fix proposed in #703 and nothing beyond it.
What I checked
- CI: all green on 24fe003. The earlier README-check failure in run 41716837216 was re-run and now passes. The author's diagnosis, that the CDN was serving an updated modules file, fits: that check generates the README from the downloaded file, and this change doesn't alter how that file is downloaded.
- Tests: I ran
tests/models/test_modules.py. All 5 pass with the change. Withlean/models/__init__.pyreverted to master, the 2 new tests fail. So they cover both new paths: an empty placeholder gets replaced by a download, and an empty placeholder plus a failed download raisesModules json not foundinstead of a JSON parse error. - End to end, reproduced myself: I built a wheel and an sdist from the head commit with
python -m build. Both contain a 0-bytelean/modules-1.15.json. I installed the wheel in a fresh venv.import lean.modelsdownloaded the file (201672 bytes, 37 modules), andlean --versionworked.pip uninstall -y leanthen removedsite-packages/leanentirely, where master leaves it behind. - Callers:
UpdateManager.force_refresh_modules_jsoncalls_download_modules_jsondirectly and still overwrites the placeholder as before.lean.models.cliandlean.models.cloudonly readjson_modules. - Portable build:
scripts/main.specdoesn't collect package data, so the portable build is unaffected. If it ever did collect it, an empty file now just triggers a download. - Upgrades: between two releases that use the same file name, pip removes the recorded file and installs the empty placeholder, which then gets downloaded again. Upgrading from a release before this one still leaves its file behind once, as the description says.
- Concurrency: the download still writes in place, not atomically. A second process that reads the file mid-write now retries the download when it sees 0 bytes, so that case is no worse than on master.
- The branch is a few commits behind master, but none of those commits touch the changed files.
Non-blocking: see the inline note on the package_data glob.
Other repositories: none need an update.
Risk: I set risk:low. It's a small packaging and loading change that's easy to revert. It doesn't change any CLI command or output, and a non-empty downloaded file is handled exactly as before. Both new paths have tests that fail without the fix, CI is green, and I verified the install/uninstall cycle end to end from a built wheel.
| packages=find_packages(include=["lean", "lean.*"]), | ||
| package_data={ | ||
| "lean": ["ssh/*"] | ||
| "lean": ["ssh/*", "modules-*.json"] |
There was a problem hiding this comment.
Non-blocking: the glob also ships any older modules-*.json left over in the build tree. Only the current name gets truncated to an empty placeholder, so a file left from an earlier version would be shipped with its contents. I confirmed this: building with a leftover lean/modules-1.14.json packed it, contents and all, into both the wheel and the sdist. Release builds start from a fresh checkout, and uninstall removes the file anyway, so this doesn't matter in practice. Still, ["ssh/*", modules_file_name] would ship exactly the placeholder this code writes.
There was a problem hiding this comment.
Done in the latest commit: package_data now lists modules_file_name instead of the modules-*.json glob. I built with a leftover lean/modules-1.14.json and a non-empty lean/modules-1.15.json in the tree. The wheel and the sdist now contain only lean/modules-1.15.json, at 0 bytes. Installing that wheel in a fresh venv still downloaded and loaded the 37 modules, and pip uninstall removed site-packages/lean completely.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the review note: |
There was a problem hiding this comment.
Requesting changes on df7dbbe for one reproduced regression: a metadata-only setup command destroys the downloaded modules cache, leaving a previously working editable CLI unable to start offline. Details and the requested regression test are inline.
The install/uninstall fix itself works, and the latest change resolves the earlier package-data glob concern. I independently verified:
- All CI checks completed successfully: 66 passed, 6 skipped.
- The modules and update-manager tests pass: 28 passed. The full suite with this branch's declared pytest 7 dependency passes: 2786 passed, 82 skipped. An initial run with pytest 9 hit the existing
test_get_loggerincompatibility already fixed on master by #702; that is not introduced here. - Both
python -m buildand the release-stylepython setup.py sdist bdist_wheelproduce a wheel and sdist containing only the current, zero-byte modules placeholder. The release-style build was also checked with a populated current cache and an older modules file in the source tree. - Tested end to end in a fresh virtual environment: install the wheel, download and load 37 modules, run
lean --version, uninstall, then install editable. Uninstall removessite-packages/leancompletely, and the editable CLI resolves to the checkout and works outside it. - With CDN access unavailable,
python -m lean --helpsucceeds with the populated source cache, but fails afterpython setup.py --versionon this branch. The same sequence succeeds before and after on master.
Other repositories: no coordinated code updates identified.
Risk: I set risk:high, because an operation that previously preserved a usable offline installation now invalidates it. Preserve the working source cache during metadata/build operations and cover that path while retaining the empty artifact and uninstall guarantees.
| # An empty placeholder for the modules json the CLI downloads into its package makes pip record it, so uninstalling | ||
| # removes it. The name is read rather than imported, as importing lean.models downloads the file | ||
| modules_file_name = re.search(r"^file_name = \"([^\"]+)\"", read("lean/models/__init__.py"), re.M).group(1) | ||
| with open(os.path.join(current_dir, "lean", modules_file_name), "w"): |
There was a problem hiding this comment.
[P2] Preserve the live cache when generating package metadata. This top-level write runs for python setup.py --version too, not just a package build. In an editable checkout with a downloaded modules file, I reproduced that command reducing the cache from 201672 bytes to 0. With CDN access unavailable, python -m lean --help succeeds before the command and then fails with FileNotFoundError: Modules json not found; master preserves the cache and succeeds both times. Please generate the empty distribution placeholder without clobbering the working checkout's cache (for example, in the artifact staging tree), and add a regression test proving metadata generation preserves an existing usable cache while the wheel/sdist still contain an empty placeholder.
There was a problem hiding this comment.
Fixed in 0170c9e. setup.py no longer writes anything when it loads. A build_py subclass writes the empty placeholder into build_lib after the normal build, so it lands only in the build output, and package_data is back to ["ssh/*"]. Editable builds skip it.
python setup.py --versionin the checkout left the downloaded file at 201672 bytes.python setup.py sdist bdist_wheel,pip install -e .andpip install -e . --config-settings editable_mode=strictall left a populated source file unchanged.- The wheel, and a wheel built from the sdist, contain a 0-byte
lean/modules-1.15.jsonthat is listed inRECORD. Installing then uninstalling, from the wheel or from the sdist, removessite-packages/leancompletely.
Regression tests are in tests/test_setup.py. Metadata generation and a wheel build keep an existing cache, and both wheels record an empty placeholder. The first two fail against df7dbbe. The sdist itself no longer carries the placeholder; that is not needed for uninstall, because any install from it builds a wheel through build_py.
There was a problem hiding this comment.
Requesting changes on df7dbbe. This replaces my approval of 24fe003.
What changed since my last review: package_data now lists only modules_file_name, which resolves my non-blocking glob note. Good.
Blocking: setup.py empties the checkout's live modules cache every time it runs. The placeholder is written at module level into the source tree. So it runs for every setup.py invocation, not just a build: python setup.py --version, pip install -e . and any metadata query. I reproduced this myself on the head commit with a fresh clone and an editable install:
import lean.modelsdownloadslean/modules-1.15.json(201672 bytes, 37 modules).python setup.py --version(orpip install -e .again) truncates it to 0 bytes.- With the CDN unreachable,
python -m lean --helpthen fails withFileNotFoundError: Modules json not found.
The same sequence on master keeps the 201672-byte file, and lean --help works offline. Online, the only cost is a re-download. But this adds a new way for a working editable CLI to stop. I reached this conclusion independently, and it matches the other review on this commit.
Requested: write the empty placeholder into the build output rather than the source tree. For example, use a build_py subclass that writes <build_lib>/lean/<modules_file_name> after run(), plus an sdist override of make_release_tree that writes it into the release tree. Register both via cmdclass. The wheel's RECORD covers everything in build_lib, so uninstall still removes the file, and the artifacts stay empty even when the checkout has a downloaded copy. Please add a test that covers both sides: building leaves an existing non-empty cache in the source tree untouched, and the built wheel/sdist still contain the 0-byte placeholder.
Still fine from my previous review: the lean/models/__init__.py change and its two tests, the callers (UpdateManager.force_refresh_modules_json, lean.models.cli, lean.models.cloud), the portable build (scripts/main.spec doesn't collect package data) and the upgrade behaviour.
CI: all checks pass on df7dbbe (66 passed, 6 skipped).
Other repositories: none need an update.
Risk: risk:high stays. The change, as it stands, newly breaks offline use of an editable checkout after any setup.py run. Once the placeholder only goes into build artifacts, and a test shows the source cache survives, this is a small, revertible packaging fix again.
| # An empty placeholder for the modules json the CLI downloads into its package makes pip record it, so uninstalling | ||
| # removes it. The name is read rather than imported, as importing lean.models downloads the file | ||
| modules_file_name = re.search(r"^file_name = \"([^\"]+)\"", read("lean/models/__init__.py"), re.M).group(1) | ||
| with open(os.path.join(current_dir, "lean", modules_file_name), "w"): |
There was a problem hiding this comment.
This truncates the checkout's downloaded lean/modules-<version>.json on every setup.py run, including --version and pip install -e .. Reproduced: 201672 bytes → 0, then python -m lean --help fails offline with Modules json not found, where master works. Please write the empty file into the build output instead, for example from a build_py subclass into self.build_lib and from an sdist make_release_tree override into the release tree, so the source cache is never touched.
There was a problem hiding this comment.
Done across 0170c9e and be6a506:
setup.pyno longer writes anything when it loads.- A
build_pysubclass writes the 0-byte placeholder intobuild_libafterrun(), and skips editable builds. - An
sdistoverride ofmake_release_treewrites it into the release tree. - Both are registered through
cmdclassand sharewrite_modules_placeholder. It removes any existing copy before writing, becausemake_release_treemay hard link source files, so the checkout's file is never truncated through a link.
Tests in tests/test_setup.py, each run on a source tree holding a populated modules json:
--version,bdist_wheelandsdisteach leave that file untouched.- The wheel contains and records the 0-byte placeholder.
- The sdist contains the 0-byte placeholder.
- A wheel built from the sdist records it too.
Verified in a fresh venv with pip 26.3: python -m build left a populated source file intact and produced both artifacts with the 0-byte placeholder. Installing then uninstalling either one removed site-packages/lean completely, and pip install -e . kept the source file. python static_analysis.py: BUILD PASSED. python -m pytest: 2790 passed, 82 skipped.
There was a problem hiding this comment.
Requesting changes on 9968c40 for one remaining source-cache preservation case. The direct in-place and dry-run cases from my previous review are fixed, and the new tests correctly reject the earlier implementation.
Blocking: the guard compares the build root with the checkout root, rather than comparing the actual package directories. If build/lib/lean links to the checkout's lean directory, an ordinary python setup.py build_py succeeds but replaces the live modules cache with an empty file. I reproduced 201672 bytes → 0, followed by python -m lean --help failing when downloads are unavailable. Master preserves the cache and works offline in the same scenario. This also reproduces with setuptools 84.2.1. The inline comment includes the reproduction and requested regression coverage.
Verified on this head:
- Focused packaging, modules and update-manager tests: 35 passed. Full suite with the branch's declared pytest dependency: 2793 passed, 82 skipped. Static analysis passed. The first full-suite attempt reached its execution time limit; the rerun above completed successfully.
- All three new in-place/dry-run cases fail against
be6a506and pass here. With setuptools 84.2.1, the packaging/modules tests give 10 passed, 2 skipped, correctly skipping the removed dry-run option. - Direct in-place builds, a symlink to the whole checkout, and a hard-linked build-cache file preserve the source cache. A separate preservation test for a symlinked package directory passes on master and fails on this head.
- Tested end to end with Python 3.11 and pip 26.3:
python -m buildpreserves the source cache; wheel and sdist contain only the current empty placeholder, and the wheel records it. Installing either artifact downloads and loads 37 modules,lean --versionworks, and uninstall removessite-packages/leancompletely. A subsequent editable install resolves to the checkout and works outside it.
CI: the push Build and the rerun of Regression Testing passed. The pull-request Build remains red at Check README in test (ubuntu-24.04, 3.9); 15 other jobs were cancelled by fail-fast. I compared its logged README patch with master's failed Build on f0e7416: they are identical, adding the catalog's Interactive Brokers (FIX) options. This is the already-reported baseline README drift, not a blocker introduced by this PR.
Other repositories: no coordinated code updates identified.
Risk: I set risk:high and removed risk:low. A linked output package can still destroy a previously usable offline checkout's cache. Compare the resolved destination package directory with the source package directory and cover that case while retaining the verified artifact/uninstall behavior.
| """Writes an empty placeholder for the modules json the CLI downloads into its package, so pip records the file and | ||
| uninstalling removes it. Only build outputs get it, keeping a checkout's downloaded file intact.""" | ||
| # A dry run changes nothing, and an in-place build's package is the checkout itself | ||
| if command.dry_run or os.path.samefile(package_root, current_dir): |
There was a problem hiding this comment.
[P2] Compare the destination package directory, not only its parent. The new guard misses an output tree whose lean directory is a symlink to the checkout's lean directory. In a clean checkout with a populated modules cache, this reproduces with:
mkdir -p build/lib
ln -s "$PWD/lean" build/lib/lean
python setup.py build_pybuild/lib and the checkout are different directories, so samefile returns false, but removing/writing build/lib/lean/modules-1.15.json operates on the live source cache. I reproduced a successful build changing that file from 201672 bytes to 0 on both setuptools 80.9.0 and 84.2.1. With downloads unavailable, python -m lean --help succeeds before the build and fails afterward with Modules json not found. The identical sequence on master preserves the cache and works offline.
Please compare the actual destination lean directory with the source lean directory before replacing the cache, and add a regression test for a linked package directory (not just a linked build root). The standalone preservation test I ran passes on master and fails on this head.
There was a problem hiding this comment.
Fixed in 5fe501b. The guard now compares the destination package folder with the checkout's: os.path.samefile(os.path.join(package_root, "lean"), os.path.join(current_dir, "lean")). That covers your linked build/lib/lean and the earlier in-place --build-lib . with one check. The placeholder path is built from that same folder.
New test test_setup_build_into_linked_package_keeps_downloaded_modules_json: it symlinks build/lib/lean to the source tree's lean, runs setup.py build_py, and asserts the populated modules json is intact. It fails against 9968c40 and passes here. It's skipped on Windows, where creating symlinks may need elevated rights. The existing in-place, dry-run and artifact tests still pass.
python -m build still leaves the source file intact, the wheel records a 0-byte lean/modules-1.15.json, and the sdist contains it. Installing then uninstalling either one in a fresh venv removes site-packages/lean completely. python static_analysis.py: BUILD PASSED. python -m pytest: 2794 passed, 82 skipped.
A build output whose lean folder links to the checkout's skipped the earlier guard, which compared only the build roots. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the latest review in 5fe501b. The placeholder guard now compares the destination |
|
CI on 5fe501b: The README diff is the same one as before: I couldn't re-run these jobs: both runs were still in progress, and GitHub answered |
There was a problem hiding this comment.
Approving 5fe501b. This replaces my approval of 9968c40.
What changed since my last review: the guard in write_modules_placeholder now checks the destination lean package folder against the checkout's lean folder (os.path.samefile), instead of checking the build roots. The placeholder path is built from that same folder. A new test symlinks build/lib/lean to the source lean and runs build_py. That addresses the linked-package case the other review raised on 9968c40, and nothing else changed.
What I checked on this commit
-
CI: both Build runs on
5fe501bfailed only at Check README, intest (ubuntu-24.04, 3.14)(push) andtest (ubuntu-22.04, 3.10)(pull request). Fail-fast cancelled the other jobs. The README diff adds onlyInteractive Brokers (FIX)and--ib-fix-user-name/--ib-fix-account, from the updated downloadable modules file. Master's Build onf0e7416(run 41728051351) fails the same step with the same lines, so this change doesn't cause it. Run tests passed in every job that reached it: all 12 Ubuntu jobs, four macOS 14 jobs and Windows 3.13. Regression Testing passed. Fail-fast cancelled the other macOS 15 and Windows jobs before they ran tests. On those platforms the new test is either skipped (Windows) or exercises onlyos.path.samefile, and every job of the push Build on9968c40passed. -
Tests:
tests/test_setup.pyandtests/models/test_modules.pypass in a fresh venv set up fromrequirements.txt(13 passed). With9968c40'ssetup.py, the new linked-package test fails and the other 7 pass, so it catches the regression. -
Guard, reproduced in a fresh clone holding a downloaded modules file (201672 bytes): the source file is unchanged after each of these:
build_pyinto abuild/lib/leansymlinked to the checkout'sleanbuild_pyinto abuild/libsymlinked to the checkoutbuild_py --build-lib .build_py --build-lib ./lean/..python -m build- the release job's
python setup.py sdist bdist_wheel
A normal
build_pystill writes a 0-byte placeholder intobuild/lib/lean, including over an existing one. -
End to end, fresh venv: the wheel contains a 0-byte
lean/modules-1.15.jsonand lists it inRECORD. The sdist contains the same 0-byte file. After installing the wheel,import lean.modelsdownloaded the file (203620 bytes, the CDN's current copy), andlean --versionworked.pip uninstall -y leanthen removedsite-packages/leancompletely. -
New failure paths: none.
samefileneeds the destinationleanfolder to exist. It always does at both call sites: afterbuild_py.run()copies the package, and aftermake_release_treebuilds the release tree. The previous check had the same requirement on the build root. -
Unchanged from my earlier reviews: the empty-file handling in
lean/models/__init__.pyand its tests, the callers (UpdateManager.force_refresh_modules_json,lean.models.cli,lean.models.cloud), the portable build, and the upgrade trade-off (an upgrade between two releases that use the same file name re-downloads it).
Other repositories: none need an update.
Risk: I set risk:low and removed risk:high.
- The other review set
risk:highon9968c40, for the linked-package case. That case is now fixed and covered by a test that fails without the fix. - This delta only narrows which build destinations get the write, so it adds no new way to fail.
- The whole change is a small packaging fix that's easy to revert. It changes no CLI command or output, and I verified the build, install and uninstall cycle end to end.
- The other reviewer still needs to review this commit.
- The README check stays red on master and every branch until
README.mdis regenerated, and that's outside #703.
There was a problem hiding this comment.
Approving 5fe501b. This replaces my request for changes on 9968c40.
The linked-package blocker is fixed: the guard now compares the actual destination lean directory with the source package directory. I ran the new regression test unchanged against both implementations: it passes on this head and fails on 9968c40. No remaining blocking findings.
Verified on this head
- Focused packaging, modules and update-manager tests: 36 passed. Full suite with the branch's declared pytest dependency: 2794 passed, 82 skipped. Static analysis passed with five unused-import warnings in unchanged files.
- With setuptools 84.2.1, full-source builds into a linked package directory, a linked build root and the checkout itself preserve the downloaded cache. A hard-linked cache in a separate build directory is replaced without changing the source file. In every case,
python -m lean --helpstill works with downloads disabled. python -m buildpreserves the populated source cache. The wheel and sdist contain only the current empty modules placeholder, and the wheel lists it inRECORD; an older modules file in the source tree is not shipped.- Tested end to end with Python 3.11 and pip 26.3: installing either artifact downloads and loads 37 modules,
lean --versionworks, and uninstall removessite-packages/leancompletely. Subsequent default and strict editable installs preserve the source cache and work from outside the checkout.
CI: Regression Testing passed. Both Build runs remain red at Check README. I verified the author's baseline diagnosis against the failing logs: their README patches match master's failed Build on f0e7416 (run 41728051351), adding the catalog's Interactive Brokers (FIX) options. The remaining interruptions are fail-fast cancellations, not evidence that unfinished tests passed. This unrelated failure is tracked in #705 and does not block this fix.
Other repositories: no coordinated code updates identified.
Risk: retained risk:low. The source-cache regression from my previous review is fixed and covered by a test that rejects the previous implementation. This is a small, revertible packaging fix with unchanged CLI commands and output, preserved working-cache behavior during builds, and independently verified build/install/uninstall results.
|
CI on 5fe501b: The failing step's diff only adds Both runs have finished now, so I've re-run their failed jobs once. I expect Check README to fail the same way until |
|
The re-runs of the 5fe501b Build runs failed Check README again, this time in The diff is the same as before: |
Closes #703
What changed and why
The CLI downloads
modules-<version>.jsoninto its own package folder (site-packages/lean/). pip didn't install that file, sopip uninstall leanleft the file and thelean/folder behind. Python then picks the folder up as a namespace package, which shadows a later PEP 660 editable install (ImportError: cannot import name '__version__' from 'lean').Implements the fix proposed in the issue, with the placeholder written to the build output instead of the source tree:
setup.pywrites an emptylean/modules-<version>.jsoninto the build output only:build_pyadds it tobuild_libafter the normal build, andsdistadds it to the release tree. Every wheel, including one built from the sdist, contains and records that file, and the sdist contains it too. So pip removes it on uninstall, even after a download has overwritten it. Details:python setup.py --version, builds and editable installs keep it as it is, so an editable CLI keeps working offline.editable_mode), dry runs and builds whoseleanpackage folder is the checkout's own, such asbuild_py --build-lib .or abuild/lib/leansymlinked to the checkout'slean, skip the placeholder.lean/models/__init__.pywith a regex, because importing it would trigger the download.modules-*.json, doesn't ship either one.lean/models/__init__.pytreats an empty file as not downloaded. It downloads when the file is missing, empty or older than a day. When the file is still empty after a failed download, it raises its existingModules json not founderror instead of failing to parse it.Backwards compatible: a non-empty downloaded file is handled as before. Releases before this one don't record their downloaded file, so upgrading from one of them still leaves it behind once.
Tests
tests/models/test_modules.py:Modules json not found.lean/models/__init__.pychange.tests/test_setup.pyrunssetup.pyon a copy of the sources that holds a downloaded modules json:--versionleaves the file intact.build_py --build-lib ., the same command with--dry-run, and--dry-run build_pyinto a separate folder all leave the file intact, and so doesbuild_pyinto abuild/lib/leansymlinked to the checkout'slean(skipped on Windows, where creating symlinks may need elevated rights).sdist bdist_wheelproduces a wheel that contains and records an emptylean/modules-<version>.json, and leaves the source file intact.sdistproduces an sdist that contains an emptylean/modules-<version>.json, and leaves the source file intact.python static_analysis.py: BUILD PASSED (the 5 unused import warnings are in untouched files).python -m pytest -s -rs: 2794 passed, 82 skipped. Locally the checkout was onPYTHONPATHbecause my machine has a leftoversite-packages/lean/from an older release, which is exactly this bug.Reproduced and verified (fresh venv, Python 3.11, pip 26.3)
python -c "import lean.models", thenpip uninstall -y lean. This leavessite-packages/lean/modules-1.15.jsonbehind.lean/modules-1.15.jsonthat is listed inRECORD. After install,import lean.modelsdownloads the 201672-byte file and loads 37 modules, andlean --versionworks.pip uninstall -y leanremovessite-packages/leancompletely. A followingpip install -e .resolveslean.__path__to the checkout, andlean --versionworks from outside it.lean/modules-1.15.json. Installing it with pip installs the 0-byte placeholder, loads 37 modules, and uninstalling removessite-packages/lean.lean/modules-1.15.jsonin the source tree, it stays unchanged after:python setup.py --versionpython setup.py sdist bdist_wheelpip install -e ., both default andeditable_mode=strict🤖 Generated with Claude Code