`isolated_filesystem` contextmanager issues

[Karrenbelt]

auto_dev/auto_dev/utils.py

Lines 191 to 211 in 18db783

	@contextmanager
	def isolated_filesystem(copy_cwd: bool = False):
	"""Context manager to create an isolated file system.
	And to navigate to it and then to clean it up.
	"""
	original_path = Path.cwd()
	with tempfile.TemporaryDirectory(dir=tempfile.gettempdir()) as temp_dir:
	temp_dir_path = Path(temp_dir).resolve()
	os.chdir(temp_dir_path)
	if copy_cwd:
	# we copy the content of the original directory into the temporary one
	for file_name in os.listdir(original_path):
	if file_name == "__pycache__":
	continue
	file_path = Path(original_path, file_name)
	if file_path.is_file():
	shutil.copy(file_path, temp_dir_path)
	elif file_path.is_dir():
	shutil.copytree(file_path, Path(temp_dir, file_name))
	yield str(Path(temp_dir_path))
	os.chdir(original_path)

The design of this context manager is problematic in more ways than one:

• copying is slow
• sys.path not modified
• cached imports; cannot import newly scaffolded modules for testing
• changing directory, but not restoring properly (no finally clause)
• and many more!

And if all of that is not enough, let me show you something I have witnessed before and again now. When i use this fixture (or rather a derivative) and place a breakpoint in the test to debug (here after first assert result)

auto_dev/tests/test_scaffold.py

Lines 92 to 107 in 18db783

	def test_scaffold_protocol(cli_runner, dummy_agent_tim):
	"""Test scaffold protocol."""
	path = Path.cwd() / ".." / "tests" / "data" / "dummy_protocol.yaml"
	command = ["adev", "scaffold", "protocol", str(path)]
	runner = cli_runner(command)
	result = runner.execute()
	assert result, runner.output
	assert runner.return_code == 0, result.output
	assert "New protocol scaffolded" in runner.output
	protocol = read_protocol(str(path))
	original_content = path.read_text(encoding=DEFAULT_ENCODING)
	readme_path = dummy_agent_tim / "protocols" / protocol.metadata["name"] / "README.md"
	assert original_content in readme_path.read_text(encoding=DEFAULT_ENCODING)

and check where the hell i am since i seem unable to access local variables, I get

(Pdb) __file__
'/home/zarathustra/.cache/pypoetry/virtualenvs/autonomy-dev-IM6WInhx-py3.11/lib/python3.11/site-packages/_pytest/logging.py'

amazing!

Turns out test was just updated to pass again. So I go in, place breakpoint and check where i end up now!

(Pdb) __file__
'/home/zarathustra/Projects/auto_dev/auto_dev/cli_executor.py'

that is amazing! I wonder where we'll end up next time we make some changes...

Alright enough sarcasm. It is time to replace the isolated_filesystem with something better. I have gone over various designs for this and found all of them to be faulty to a degree, I will share some implementations i tried below

[Karrenbelt]

So here is an attempt I made about a year ago to try and addresses several shortcomings of the above implementation

import os
import sys
import types
import shutil
import tempfile
import importlib
import contextlib
from typing import Any, Generator
from pathlib import Path
from functools import partial
from itertools import chain

from utils.repository import get_repo_root, get_gitignore_clean


def is_submodule(parent: types.ModuleType, obj: Any) -> bool:
    if not isinstance(obj, types.ModuleType):
        return False
    module_path = Path(getattr(obj, "__file__", "") or "")
    parent_path = Path(getattr(parent, "__file__", "") or "").parent
    return module_path.is_relative_to(parent_path)


def get_submodules(module: types.ModuleType) -> list[types.ModuleType]:
    node_depth, links = {module: 0}, set()
    from_same_module = partial(is_submodule, module)

    def traverse(node: types.ModuleType, depth: int) -> None:
        for child in filter(from_same_module, vars(node).values()):
            if (link := (node, child)) in links:
                raise ValueError("Cycle detected in dependency graph!")
            links.add(link)
            node_depth[child] = max(depth, node_depth.get(child, depth))
            traverse(child, depth + 1)

    traverse(module, 0)
    return sorted(node_depth, key=lambda x: (node_depth[x], x.__file__))


def reload_modules(module_name: str) -> None:
    """Reload modules and submodules."""

    if not (module := sys.modules.get(module_name)):
        raise ValueError(f"Module '{module_name}' not found in sys.modules")

    importlib.invalidate_caches()
    from_same_module = partial(is_submodule, module)
    modules_in_scope = filter(from_same_module, sys.modules.copy().values())
    for module in chain.from_iterable(map(get_submodules, modules_in_scope)):
        importlib.reload(module)


@contextlib.contextmanager
def sandbox_repo(*ignore_patterns: str) -> Generator[Path, None, None]:
    """Setup a sandboxed repository environment."""

    repo_root = get_repo_root()
    module = sys.modules[next(iter(__package__.split(".")))]
    assert module.__file__
    subdir = os.path.relpath(module.__file__, start=repo_root).split(os.sep)[0]

    original_env = os.environ.copy()
    original_path = Path.cwd()
    original_sys_path = sys.path.copy()

    with tempfile.TemporaryDirectory() as tmp_dir:
        try:
            tmp_path = Path(tmp_dir) / repo_root.name
            ignore = shutil.ignore_patterns(*get_gitignore_clean(), *ignore_patterns)
            shutil.copytree(repo_root, tmp_path, ignore=ignore)
            sys.path.insert(0, str(tmp_path / subdir))
            reload_modules(module.__name__)
            os.chdir(tmp_path)
            yield tmp_path
        finally:
            os.chdir(original_path)
            os.environ.clear()
            os.environ.update(original_env)
            sys.path = original_sys_path
            reload_modules(module.__name__)

It attempts to reload all the modules in the correct topological order, which is crucial in order to be able to import newly scaffolded modules from the temporary directory. It does some more things, like modifying sys.path and restoring it, and restoring environmental variables. However, several issues remain, including the inability to actually end up where one places a breakpoint. Moreover, it seems overly complex to me, something of a red flag in itself.

[Karrenbelt]

Here is my current proposal for a solution (module is named "rollback")

import os
import signal
import shutil
import tempfile
from pathlib import Path
from contextlib import contextmanager

from utils import signals

# https://www.youtube.com/watch?v=0GRLhpMao3I
# async-signal safe is the strongest concept of reentrancy. 
# async-signal safe implies thread safe.

# signal.SIGKILL cannot be intercepted
SIGNALS_TO_BLOCK = (signal.SIGINT, signal.SIGTERM)


def restore_from_backup(directory: Path, backup: Path):
    for item in directory.rglob("*"):
        backup_item = backup / item.relative_to(directory)
        if item.is_file() or item.is_symlink():
            item.unlink()
        elif item.is_dir() and not backup_item.exists():
            shutil.rmtree(item)

    for item in backup.rglob("*"):
        directory_item = directory / item.relative_to(backup)
        if item.is_symlink():
            directory_item.symlink_to(item.readlink())
        elif item.is_dir():
            directory_item.mkdir(parents=True, exist_ok=True)
        elif item.is_file():
            shutil.copy2(item, directory_item)


@contextmanager
def on_exit(directory: Path):
    backup = Path(tempfile.mkdtemp(prefix="backup_")) / directory.name
    shutil.copytree(directory, backup, symlinks=True)
    try:
        yield
    finally:
        with signals.mask(*SIGNALS_TO_BLOCK):
            restore_from_backup(directory, backup)
        shutil.rmtree(backup)


@contextmanager
def on_exception(directory: Path):
    backup = Path(tempfile.mkdtemp(prefix="backup_")) / directory.name
    shutil.copytree(directory, backup, symlinks=True)
    try:
        yield
    except BaseException:
        with signals.mask(*SIGNALS_TO_BLOCK):
            restore_from_backup(directory, backup)
        raise
    finally:
        shutil.rmtree(backup)

with signals.mask looking as follows

@contextmanager
def mask(*signals: int):
    """
    Context manager to temporarily block specified signals for the current thread by modifying its signal mask.
    """
    original_mask = signal.pthread_sigmask(signal.SIG_BLOCK, signals)
    try:
        yield
    finally:
        signal.pthread_sigmask(signal.SIG_SETMASK, original_mask)

Unlike the previous two versions, this allows working on the current working directory (i.e. repository) directly. This prevents having to mess around with cached imports and path changes. I believe this should also resolve the issue with the pdb dropping you at a placec where you did not place the breakpoint.

One thing to be aware of is that it is possible to entirely nuke your repository when interrupting during the restoration process. That is exactly why SIGNALS_TO_BLOCK are defined and masked during restoration, postponing their effect until after restoration is completed. While this works we as well, it should be noted that signal.SIGKILL CANNOT be blocked.

[8ball030]

Looks absolutely lovely.
I have some issues with coverage not correctly test coverage as well, I think due to the temporary path selected.

One further issue i have noticed, is that for example, the coverage report has issues with tests on tests form the isolated file system.

This is due to the fact new temporary directories are created with the tests, and coverage tries to then parse the entire genreated copied code which is then removed after the tests so it throws a little.

I would love to actually understand how much proper test coverage we have rn as it looks like 20-30% etc.

[Karrenbelt]

well it's probably rather on the low end, not sure the isolated filesystem has much of anything to do with that to be frank, but we could test that. The isolated file system is used for testing generated code, using our current code, and the generated code is of course not part of the coverage report. In our case, which concerns effectively meta-programming, the coverage report is in fact insufficient as it does not reveal how much of the generated code is in fact covered by tests.

I have acquired some insights I'll briefly document here. As state previously, the isolated_filesystem is not a properly isolated system. This is in part revealed by failing tests when one tries to use pytest-xdist, which reveals there exist cross test interactions (failures of tests are proof of this). One issue is that the python interpreter we use has not only cached imported modules (which i tried to address with in my implementation above with module reloading), but should in fact be ran from the isolated_filesystem! This can be achieved by running it as a subprocess and passing a custom env where the PYTHONPATH is set to that env. For isolated_filesystem cases where we do not copy the entire repo, we should instead not even use this, as a the functionality of tempfile will likely suffice for these cases (we can use our main interpreter, as long as we don't modify the auto_dev repository). I commented on that in PR #618 (here).

Now this is something we can address by using monkeypatch in our fixtures. However, this does not offer us the necessary functionality in our production code. For our production code, i suggest using the roll mechanism (PR #610 & #612). This rollback mechanism will be useful in production, where we want to work on a repository directly and not even restore unless we have to. However, operating on or in the repository itself in tests is undesirable, as we won't be able to use pytest-xdist. To sum up, I think both of these are part of the solution; isolated filesystem with subprocess for tests (as we effectively want to run pytest in a pytest) and rollback for the application itself.

[Karrenbelt]

While this may not be directly related, I suspect the issues are entangled or will surface together.

In #618, attempts to create a module_scoped_dummy_agent_tim fixture based on existing ones failed. Only a direct subprocess invocation worked, suggesting a different underlying cause, possibly in the CommandExecutor.

Mentioning here because fixing isolated_filesystem might surface similar latent bugs that were previously hidden by working around them.