RFC: Add a new class to deal with some uninstallation issues#14097
RFC: Add a new class to deal with some uninstallation issues#14097vfazio wants to merge 3 commits into
Conversation
|
This will likely fail on windows because i didn't test it there and i'm sure i got the path stuff wrong somehow |
|
With apologies @notatallshaw |
|
Since this is just an RFC and probably not going to be merged as-is, I'm marking this as ready for review |
0829f13 to
7335f6b
Compare
Note that the skipped directories were added so that file skipped for a namespace package could be wildcarded for display and not give users the expectation that everything would be removed.
7335f6b to
d1bb46d
Compare
|
updated the PR description to reflect the new PR template and rebasing |
|
Just to let you know, I'm unlikely to review this since I was not involved when work was originally being done here. This is a massive PR and I have larger priorities since we are approaching release 26.2. The other maintainers who were previously involved here would be a better bet for reviews. Thanks! Also, we recently merged #14108 which probably changes your performance numbers. edit: improving phrasing since it was too brash originally |
|
Yea, no offense taken. It is large though a lot of it is comments and this is largely meant for discussion. I did try to address a few long standing issues but they've been around for so long I can understand if this overhaul is seen as not worth it. For now this doesn't change any actual code in the uninstall path and is a side-by-side implementation. The performance numbers should not be affected by any changes to |
|
rebasing and retesting diff --git a/src/pip/_internal/req/req_uninstall.py b/src/pip/_internal/req/req_uninstall.py
index 11e9aae2b..7feea0ce2 100644
--- a/src/pip/_internal/req/req_uninstall.py
+++ b/src/pip/_internal/req/req_uninstall.py
@@ -113,6 +113,30 @@ def compact(paths: Iterable[str]) -> set[str]:
return short_paths
+def compress_for_rename2(
+ paths: Iterable[str], dist: BaseDistribution | None = None
+) -> set[str]:
+ roots = None
+ if dist and dist.installed_location:
+ roots = [dist.installed_location]
+ pc = PathCompactor(paths=paths, preserved_roots=roots)
+ pc._parse_paths()
+ pc._calculate_roots_and_owned_paths()
+ pc._process_roots()
+ return pc.compress_for_rename()
+
+def compress_for_output_listing2(
+ paths: Iterable[str], dist: BaseDistribution | None = None
+) -> tuple[set[str], set[str]]:
+ roots = None
+ if dist and dist.installed_location:
+ roots = [dist.installed_location]
+ pc = PathCompactor(paths=paths, preserved_roots=roots)
+ pc._parse_paths()
+ pc._calculate_roots_and_owned_paths()
+ pc._process_roots()
+ return pc.compress_for_output_listing()
+
def compress_for_rename(paths: Iterable[str]) -> set[str]:
"""Returns a set containing the paths that need to be renamed.
@@ -396,8 +420,14 @@ class UninstallPathSet:
for path in sorted(compact(paths)):
logger.info(path)
+ import time
if not verbose:
+ t0 = time.monotonic()
will_remove, will_skip = compress_for_output_listing(self._paths)
+ print(time.monotonic() - t0)
+ t0 = time.monotonic()
+ will_remove, will_skip = compress_for_output_listing2(self._paths)
+ print(time.monotonic() - t0)
else:
# In verbose mode, display all the files that are going to be
# deleted.
@@ -407,8 +437,15 @@ class UninstallPathSet:
_display("Would remove:", will_remove)
_display("Would not remove (might be manually added):", will_skip)
_display("Would not remove (outside of prefix):", self._refuse)
+
+ t0 = time.monotonic()
+ paths = compress_for_rename(self._paths)
+ print(time.monotonic() - t0)
+ t0 = time.monotonic()
+ paths = compress_for_rename2(self._paths)
+ print(time.monotonic() - t0)
if verbose:
- _display("Will actually move:", compress_for_rename(self._paths))
+ _display("Will actually move:", paths)
return ask("Proceed (Y/n)? ", ("y", "n", "")) != "n"Crude timings for differences between Second number is the new code. Perf doesn't look impacted by recent changes on my current setup. Its probably important to call out that these are not scientific tests and may be subject to things like filesystem cache hits. It's also important to note that with the new class, the search results can be cached so the cost of the search could only happen once. The only tax is calculating the results from that search which shouldn't be overly expensive. In the current code we have the following search hits: If not verbose:
If verbose:
Both paths call So, we could save one, maybe two, directory scans at minimum, though like i mentioned in the description, the current algorithm calls |
What does this PR do?
Related discussions:
This is a draft PR to have a discussion about deprecating
compress_for_renameand possiblycompress_for_output_listing.The
PathCompactor(working title... i'll openly admit that I was half-inspired by the garbage compactor in Star Wars ) added in this PR takes a different approach to identifying files compared tocompress_for_rename.Notables:
__pycache__to the list of paths for removal)bin/and<purelib>/directories when a package is the final package in an installation path (Likely to be the distribution'sinstalled_location)compress_for_renamewhich will walk all directories potentially multiple times in an attempt to create wildcards.compress_for_rename<purelib>/(yes, you setuptools and six and others) do not force anos.walkof all subdirectories within; path searches are jailed based on the paths received from the path list (RECORD)I've spent too many hours writing and testing and thinking about this since April/May out of guilt over the previous attempt to solve the
__pycache__issue so I'm mostly pushing this so I can get it out of my head and let it bake.The risk here is that this is a major overhaul, so really does need time to be tested by others besides me and some thorough review by a maintainer. The behavior of
compress_for_output_listingis shaky, but maybe it can be updated to work better... the namespace scenarios are the hardest cases here which is not accounted for in the old code.I've tried to account for most production scenarios I can think of via the unit test matrix, but i'm sure i've missed some edge cases... I tried to be extremely thorough in these tests so as to:
The calling interface is not finalized because I wanted input on how to handle it. The expectation is that a
PathCompactor, or w/e we call it, is constructed once and is reused but never modified. The results of the public functions should be cacheable to avoid calculation overhead, but these are only called in a handful of places and pure string manipulation and set operations are relatively quick.I've been comparing performance with the original implementation mostly via something like the below:
Performance on a handful of packages:
PR Checklist: