Remove __pycache__ when package is removed

[vfazio]

Related to #11835

This PR has been updated to completely remove the adjacent __pycache__ when a file is selected for removal during package uninstall.

If we find there are issues with not be selective when nuking this directory (due to maybe nuking pyc for packages sharing the same namespace), we can revert to a more conservative approach of including pyc files for each possible optimization level (the initial draft of this PR went this route), but that will not cover scenarios of remnant pyc files having been generated by multiple interpreter versions (as we will only uninstall for the currently active interpreter version and the version used during installation if bytecode was compiled as part of installation).

We pivoted to this method because uv seems to nuke the entire __pycache__ directory with little ill effect.

This should partially address the scenario where a module has been loaded by an interpreter with various optimization levels and has generated multiple pyc files.

This does not resolve scenarios where pyc files have been generated by multiple versions of an interpreter or multiple interpreter implementations.

For scenarios where there are files for multiple versions or interpreters, explicit virtual environments may be a more correct solution since dependency trees change based on interpreter and version.

Otherwise, short of nuking all of __pycache__ which may affect cached bytecode for packages sharing a namespace or moving it out of the package install directory, this seems like a reasonable stop gap for this specific situation.

[pfmoore]

Hard-coding the optimisation levels like this seems fragile. While it's unlikely the valid values will change, can we not get them from the import system somehow, so we know they are correct?

[vfazio]

They are unfortunately not enumerated anywhere that can be imported.

They basically map to:

• "" = python or PYTHONOPTIMIZE=0 python
• 1 = python -O or PYTHONOPTIMIZE=1 python
• 2 = python -OO or PYTHONOPTIMIZE=2 python

https://github.com/python/cpython/blob/3c4429f65a894af2a0aea2aeed5f61bc399e5af5/Lib/importlib/_bootstrap_external.py#L239

[pfmoore]

We should at least include a comment linking back to where these are stated as being the valid values. That shouldn't be the source code of importlib, it should be PEP 488 (assuming that's the definitive location).

By the way, for comparison, uv seems to remove the whole __pycache__ directory. I installed packaging, imported it, then created a file called HELLO in site-packages/packaging/__pycache__, then uninstalled packaging, and the file HELLO was deleted as part of the uninstall. I don't know how I feel about that, but it's something to note in terms of prior art.

[vfazio]

I did note uv's behavior in #11835 (comment)

It does seem problematic that:

ns-package-1: a/b/file.py
ns-package-2: a/b/file2.py

will both utilize a/b/__pycache__ and the optimized files will be nuked if if either of those packages get reinstalled. Subsequent module loading should recompile the bytecode however.

This change seemed like the less intrusive option. I don't know if other tools utilize __pycache__. I think there was some discussion about that in the linked issue.

Since pip is utilized by a lot of people, I wanted to take a conservative approach here. But if the maintainers feel it's appropriate to nuke __pycache__ wholesale, I can change the code.

I will update the commit to link to some documentation about the optimization levels.

[vfazio]

It suppose it could be argued that anything in __pycache__ is null and void if the package is uninstalled, especially if the expectation is that the interpreter or other mechanism can just regenerate the optimized/cached files.

__pycache__ should only be generated for .py files without a side-by-side .pyc I believe, so the removal of any .py specified in a RECORD should maybe signal that __pycache__, if it exists, should be removed?

Open to thoughts

[notatallshaw]

When I have a moment I'll investigate if uv has faced any issues with that approach. I quite like the elegance and simplicity deleting the directory rather than the individual files.

[notatallshaw]

I haven't found any user reported issues to do with __pycache__ being deleted: https://github.com/astral-sh/uv/issues?q=is%3Aissue%20%22uv%20pip%20uninstall%22%20%22__pycache__%22.

I also asked the devs on the discord and they don't remember any reported issues.

I'm going to take a look at the code this week, but I'm currently leaning toward taking that approach, to me it seems simpler and uv shows it is battle tested.

[vfazio]

Thanks for the update.

My preliminary diff for nuking __pycache__ is something along the lines of:

diff --git a/src/pip/_internal/req/req_uninstall.py b/src/pip/_internal/req/req_uninstall.py
index 3f3dde2fd..bcc245903 100644
--- a/src/pip/_internal/req/req_uninstall.py
+++ b/src/pip/_internal/req/req_uninstall.py
@@ -338,7 +338,16 @@ class UninstallPathSet:
         # __pycache__ files can show up after 'installed-files.txt' is created,
         # due to imports
         if os.path.splitext(path)[1] == ".py":
-            self.add(cache_from_source(path))
+            pycache = os.path.join(os.path.dirname(path), "__pycache__")
+            # only evaluate once if it exists
+            if pycache not in self._paths | self._refuse:
+                self.add(pycache)
+        elif os.path.basename(path) == "__pycache__":
+            for dirpath, _, dirfiles in os.walk(path):
+                # if __pycache__ is permitted then files within should be too so
+                # skip the recursive call and add files directly to self._paths
+                for fname in dirfiles:
+                    self._paths.add(os.path.join(dirpath, fname))
 
     def add_pth(self, pth_file: str, entry: str) -> None:
         pth_file = self._normalize_path_cached(pth_file)

I think this should take advantage of existing code for collapsing directories and removing them via a rmtree

(xtf-py3.11) root@rpi-6a0052:/opt/xtf# ls /opt/xtf/.venv/lib/python3.11/site-packages/sniffio/__pycache__
_impl.cpython-311.opt-1.pyc  _impl.cpython-311.opt-2.pyc  _impl.cpython-311.pyc  __init__.cpython-311.opt-1.pyc  __init__.cpython-311.opt-2.pyc  __init__.cpython-311.pyc  _version.cpython-311.opt-1.pyc  _version.cpython-311.opt-2.pyc  _version.cpython-311.pyc
(xtf-py3.11) root@rpi-6a0052:/opt/xtf# pip -vvvvvv uninstall sniffio
Found existing installation: sniffio 1.3.1
Uninstalling sniffio-1.3.1:
  Would remove:
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio-1.3.1.dist-info/INSTALLER
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio-1.3.1.dist-info/LICENSE
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio-1.3.1.dist-info/LICENSE.APACHE2
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio-1.3.1.dist-info/LICENSE.MIT
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio-1.3.1.dist-info/METADATA
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio-1.3.1.dist-info/RECORD
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio-1.3.1.dist-info/REQUESTED
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio-1.3.1.dist-info/WHEEL
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio-1.3.1.dist-info/top_level.txt
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio/__init__.py
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio/__pycache__
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio/_impl.py
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio/_tests/__init__.py
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio/_tests/__pycache__
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio/_tests/test_sniffio.py
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio/_version.py
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio/py.typed
  Will actually move:
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio-1.3.1.dist-info/
    /opt/xtf/.venv/lib/python3.11/site-packages/sniffio/

[notatallshaw]

Thanks for the PR, I meant to reply when you opened it sorry, be aware all maintainers are currently volunteers donating their spare time to pip, and it's current the holiday season for a lot of them, so it may take a bit before someone has a chance to review.

[vfazio]

@notatallshaw No problems at all. Heck, I only have time to do this because it's the end of the year and i have some spare cycles myself so no rush on my part, really.

I think the biggest thing is deciding how we want pip to react in this scenario. This PR is pretty focused on a single known case, but if we wanted to mirror how uv is doing things and just blow away __pycache__ when a package is removed with the expectation that the interpreter or other mechanism regenerates files as necessary, I think that may be reasonable too but does have more implications for consumers of the files in that directory (which shouldn't be reliable after a package has been removed).

[vfazio]

I'll leave this in draft for the time being. I have updated the code with some code comments for the "focused" approach. Should we decide to pivot and nuke __pycache__, I will update the PR as appropriate.

[vfazio]

I guess one other thing is whether we need to continue to support implicit pyo file removal. These files were deprecated as of 3.5 in PEP-488. I suppose there could be pure python wheels made prior to 3.5 that could still be installed that may ship with legacy pyo files so they should still be candidates for removal, but if the wheel includes them, they should be in the RECORD and already be candidates for removal? Modules compiled via compileall should no longer generate pyo files and wheels installed when pip is given --compile will not generate pyo and the files they do generate are entered into RECORD and should be queued for removal explicitly.

There may be edge cases i'm not aware of, i'm not familiar with pre-wheel distributions.

[vfazio]

Doing a blind push for __pycache__ removal since I can't get the tests to run locally

[vfazio]

If this is deemed adequate, I can renumber the news article to reference #11835

[vfazio]

I didn't call out the scenario of byte-compiled files from other versions of the interpreters in the commit message, but I'm calling that out here in the PR. That scenario also should not be problematic, with files being regenerated the next time the interpreter loads the module.

[vfazio]

I realize everyone is busy this month, but I want to check in to see if there's anything else I can provide for this PR that would be useful.

[notatallshaw]

@vfazio I do want to carefully review and consider this PR, and the different approaches, but I am not going to have time before 26.0 is released at this end of this month. So for me the earliest I'll get chance some time in February. Apologies for the slow pace, but just the situation we're in with the few active maintainers only able to donate their spare time.

[vfazio]

@vfazio I do want to carefully review and consider this PR, and the different approaches, but I am not going to have time before 26.0 is released at this end of this month. So for me the earliest I'll get chance some time in February. Apologies for the slow pace, but just the situation we're in with the few active maintainers only able to donate their spare time.

💯 understand. I just want to make sure if there's something i need to provide that I do so and to subtly bump the PR. I've had a few PRs go stale simply because i wasn't proactive enough in bumping the changes, so i try to nudge up until i'm just shy of being annoying

[vfazio]

Just rebasing and rewording the commit message.

[notatallshaw]

The fault is mine, I didn't fully engage with the full PR (I'm trying to crank out multiple reviews this evening).

Does this mean test_compressed_listing is not behaving as expected currently even without my changes?

test_compressed_listing on main isn't affected, its sample has individual .pyc paths, never a __pycache__ directory.

Is the path argument to UninstallPathSet.add supposed to be an individual file path (is_file) and not a folder (meaning that adding pycache is not really supported?

add() already accepts directories, as stash() branches on os.path.isdir. It also guards symlinks (isdir and not islink), so a symlinked __pycache__ is stashed as a file. No need to revert to walking __pycache__ and adding files individually in add().

If path can be a directory, should compress_for_output_listing be performing an is_dir check to add these paths to the set of folders?

Don't merge these into folders. That set means "package roots to walk"; putting __pycache__ there would make the walker descend into it and emit wrong output. A separate set avoids touching the existing logic.

I don't know if there are weird situations we need to be aware of like symlinks pointing to directories and needing to use realpath?

No extra realpath needed, add() already runs normalize_path (which calls realpath) on the directory part, so _paths and the paths os.walk yields are already canonical.

Searching through all paths for each entry in paths is quadratic in the worst case scenario (I think?), so I assume we should avoid that.

Yeah, my suggestion was quadratic, you can instead prune os.walk via subdirs[:]:

_scheduled_dirs = {
    os.path.normcase(p) for p in will_remove
    if os.path.isdir(p) and not os.path.islink(p)
}

for folder in folders:
    for dirpath, subdirs, dirfiles in os.walk(folder):
        subdirs[:] = [
            d for d in subdirs
            if os.path.normcase(os.path.join(dirpath, d))
            not in _scheduled_dirs
        ]
        for fname in dirfiles:
            ...  # existing body unchanged

Like I said though, this is minor it's edge case behavior and currently untested, we can handle this in a follow up PR if it's needed.

Your diff looks close but you could use subdirectory pruning and you shouldn't be manually handling os.sep, pathlib.Path normalizes trailing slashes and os.path.isdir handles them either way.

[vfazio]

Late night drivel -

Might be worth a new issue then... add may accept paths and stash handles the paths returned from compress_for_rename but that function performs a split on the path that drops the directory name because there is no trailing slash

def compress_for_rename(paths: Iterable[str]) -> set[str]:
    """Returns a set containing the paths that need to be renamed.

    This set may include directories when the original sequence of paths
    included every file on disk.
    """
    case_map = {os.path.normcase(p): p for p in paths}
    remaining = set(case_map)
    unchecked = sorted({os.path.split(p)[0] for p in case_map.values()}, key=len)  # the final path component is dropped since directories have no trailing slash in `paths`

It seems then the directory suffix is treated as a file since its parent is used in the loop.

Also, compress_for_rename returns wildcards as ending with a os.path.sep and I think compact uses that as a marker for collapsing paths.

My quick and unoptimized solution is:

(.venv) vfazio@Zephyrus:~/development/pip$ PAGER= git diff
diff --git a/src/pip/_internal/req/req_uninstall.py b/src/pip/_internal/req/req_uninstall.py
index adb215c35..9bdc4afcc 100644
--- a/src/pip/_internal/req/req_uninstall.py
+++ b/src/pip/_internal/req/req_uninstall.py
@@ -114,12 +114,31 @@ def compress_for_rename(paths: Iterable[str]) -> set[str]:
     """
     case_map = {os.path.normcase(p): p for p in paths}
     remaining = set(case_map)
-    unchecked = sorted({os.path.split(p)[0] for p in case_map.values()}, key=len)
     wildcards: set[str] = set()
 
     def norm_join(*a: str) -> str:
         return os.path.normcase(os.path.join(*a))
 
+    # select all files in directories scheduled for removal as a first sweep
+    _dirs = {
+        key: path
+        for key, path in case_map.items()
+        if os.path.isdir(path) and not os.path.islink(path)
+    }
+
+    for key, path in _dirs.items():
+        _files: set[str] = set()
+        for dirname, _, files in os.walk(path):
+            _files.update(norm_join(path, dirname, f) for f in files)
+        remaining.difference_update(_files)
+        # remove the directory from the list of remaining items to process
+        # and add it to the list of directory wildcards
+        remaining.remove(key)
+        case_map.pop(key, None)
+        wildcards.add(path + os.sep)
+
+    unchecked = sorted({os.path.split(p)[0] for p in case_map.values()}, key=len)
+
     for root in unchecked:
         if any(os.path.normcase(root).startswith(w) for w in wildcards):
             # This directory has already been handled.
@@ -157,7 +176,12 @@ def compress_for_output_listing(paths: Iterable[str]) -> tuple[set[str], set[str
     # Determine folders and files
     folders = set()
     files = set()
+
+    _scheduled_dirs = set()
+
     for path in will_remove:
+        if os.path.isdir(path) and not os.path.islink(path):
+            _scheduled_dirs.add(os.path.normcase(path))
         if path.endswith(".pyc"):
             continue
         if path.endswith("__init__.py") or ".dist-info" in path:
@@ -171,7 +195,13 @@ def compress_for_output_listing(paths: Iterable[str]) -> tuple[set[str], set[str
     # This walks the tree using os.walk to not miss extra folders
     # that might get added.
     for folder in folders:
-        for dirpath, _, dirfiles in os.walk(folder):
+        for dirpath, subdirs, dirfiles in os.walk(folder):
+            subdirs[:] = [
+                d
+                for d in subdirs
+                if os.path.normcase(os.path.join(dirpath, d)) not in _scheduled_dirs
+            ]
+
             for fname in dirfiles:
                 if fname.endswith(".pyc"):
                     continue
diff --git a/tests/unit/test_req_uninstall.py b/tests/unit/test_req_uninstall.py
index abb5fa2d0..f9e9654ed 100644
--- a/tests/unit/test_req_uninstall.py
+++ b/tests/unit/test_req_uninstall.py
@@ -72,6 +72,7 @@ def test_compressed_listing(tmpdir: Path) -> None:
             "lib/mypkg/__init__.py",
             "lib/mypkg/my_awesome_code.py",
             "lib/mypkg/__pycache__/my_awesome_code-magic.pyc",
+            "lib/mypkg/__pycache__/.skip.garbage",
             "lib/mypkg/support/support_file.py",
             "lib/mypkg/support/more_support.py",
             "lib/mypkg/support/would_be_skipped.skip.py",
@@ -87,6 +88,14 @@ def test_compressed_listing(tmpdir: Path) -> None:
 
     # Remove the files to be skipped from the paths
     sample = [path for path in sample if ".skip." not in path]
+    sample.extend(
+        in_tmpdir(
+            [
+                "lib/mypkg/__pycache__",
+                "lib/mypkg/support/__pycache__",
+            ]
+        )
+    )
 
     expected_remove = in_tmpdir(
         [

[notatallshaw]

Yeah, it's edge case enough and could do with a review of the logic that let's make that it's own issue/PR.

[notatallshaw]

Thanks for your PR and time and patience sticking with it.

[vfazio]

@notatallshaw @pfmoore thanks for taking the time to review this and engaging with an amateur!

[vfazio]

Seems like I actually broke something with this pypa/virtualenv@6bf6716