Skip to content

refactor(bootstrapper): replace recursion with trampoline for iterative execution #1109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rd4398 wants to merge 1 commit into
base: main
Choose a base branch
from
+58 −36
Open

refactor(bootstrapper): replace recursion with trampoline for iterative execution #1109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ dependencies = [
"stevedore",
"tomlkit",
"tqdm",
"trampoline",
"wheel",
"uv>=0.8.19",
"uvicorn",
Expand Down Expand Up @@ -207,7 +208,7 @@ exclude = [

[[tool.mypy.overrides]]
# packages without typing annotations and stubs
module = ["hatchling", "hatchling.build", "license_expression", "pyproject_hooks", "requests_mock", "resolver", "spdx_tools.*", "stevedore"]
module = ["hatchling", "hatchling.build", "license_expression", "pyproject_hooks", "requests_mock", "resolver", "spdx_tools.*", "stevedore", "trampoline"]
ignore_missing_imports = true

[tool.basedpyright]
Expand Down
59 changes: 34 additions & 25 deletions src/fromager/bootstrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from packaging.utils import NormalizedName, canonicalize_name
from packaging.version import Version
from resolvelib.resolvers import ResolverException
from trampoline import trampoline

from . import (
bootstrap_requirement_resolver,
Expand Down Expand Up @@ -274,6 +275,16 @@ def _processing_build_requirement(self, current_req_type: RequirementType) -> bo
def bootstrap(self, req: Requirement, req_type: RequirementType) -> None:
"""Bootstrap a package and its dependencies.

Public entry point that drives the generator-based implementation
via trampoline() for iterative (non-recursive) execution.
"""
trampoline(self._bootstrap_gen(req, req_type))

def _bootstrap_gen(
self, req: Requirement, req_type: RequirementType
) -> typing.Generator[typing.Any, typing.Any, None]:
Copy link
Copy Markdown
Member

@dhellmann dhellmann May 4, 2026

Choose a reason for hiding this comment

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

Maybe we could be more specific with types here, instead of using typing.Any?

Copy link
Copy Markdown
Contributor Author

@rd4398 rd4398 May 4, 2026

Choose a reason for hiding this comment

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

I think we cannot be more specific because of the way the way trampoline works

- YieldType — we yield child generators, but each yield site yields a different generator type, so a single precise type isn't possible within one method 
                                                                 
- SendType — the value received from yield depends on which child generator was yielded (e.g., SourceBuildResult from _build_from_source, None from _bootstrap_gen)

Copy link
Copy Markdown
Member

@dhellmann dhellmann May 5, 2026

Choose a reason for hiding this comment

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

We can still declare our type accurately, even if trampoline doesn't care, can't we?

"""Generator that bootstraps a package and its dependencies.

Handles setup, validation, and error handling. Delegates actual build
work to _bootstrap_impl().

Expand Down Expand Up @@ -307,7 +318,9 @@ def bootstrap(self, req: Requirement, req_type: RequirementType) -> None:

# Bootstrap each resolved version
for source_url, resolved_version in resolved_versions:
self._bootstrap_single_version(req, req_type, source_url, resolved_version)
yield self._bootstrap_single_version(
req, req_type, source_url, resolved_version
)

# In multiple versions mode, report any failures for this requirement
if self.multiple_versions and self._failed_versions:
Expand All @@ -329,7 +342,7 @@ def _bootstrap_single_version(
req_type: RequirementType,
source_url: str,
resolved_version: Version,
) -> None:
) -> typing.Generator[typing.Any, typing.Any, None]:
"""Bootstrap a single version of a package.

Extracted from bootstrap() to handle both single and multiple version modes.
Expand Down Expand Up @@ -365,7 +378,7 @@ def _bootstrap_single_version(
# Track dependency chain - context manager ensures cleanup even on exception
with self._track_why(req_type, req, resolved_version):
try:
self._bootstrap_impl(
yield self._bootstrap_impl(
req, req_type, source_url, resolved_version, build_sdist_only
)
except Exception as err:
Expand Down Expand Up @@ -400,7 +413,7 @@ def _bootstrap_impl(
source_url: str,
resolved_version: Version,
build_sdist_only: bool,
) -> None:
) -> typing.Generator[typing.Any, typing.Any, None]:
"""Internal implementation - performs the actual bootstrap work.

Called by bootstrap() after setup, validation, and seen-checking.
Expand Down Expand Up @@ -453,7 +466,7 @@ def _bootstrap_impl(
)

# Build from source (handles test-mode fallback internally)
build_result = self._build_from_source(
build_result = yield self._build_from_source(
req=req,
resolved_version=resolved_version,
source_url=source_url,
Expand Down Expand Up @@ -520,9 +533,7 @@ def _bootstrap_impl(
self.progressbar.update_total(len(install_dependencies))
for dep in self._sort_requirements(install_dependencies):
with req_ctxvar_context(dep):
# In test mode, bootstrap() catches and records failures internally.
# In normal mode, it raises immediately which we propagate.
self.bootstrap(req=dep, req_type=RequirementType.INSTALL)
yield self._bootstrap_gen(req=dep, req_type=RequirementType.INSTALL)
self.progressbar.update()

# Clean up build directories
Expand Down Expand Up @@ -643,15 +654,15 @@ def _prepare_build_dependencies(
resolved_version: Version | None,
sdist_root_dir: pathlib.Path,
build_env: build_environment.BuildEnvironment,
) -> set[Requirement]:
) -> typing.Generator[typing.Any, typing.Any, set[Requirement]]:
# build system
build_system_dependencies = dependencies.get_build_system_dependencies(
ctx=self.ctx,
req=req,
version=resolved_version,
sdist_root_dir=sdist_root_dir,
)
self._handle_build_requirements(
yield self._handle_build_requirements(
req,
RequirementType.BUILD_SYSTEM,
build_system_dependencies,
Expand All @@ -667,7 +678,7 @@ def _prepare_build_dependencies(
sdist_root_dir=sdist_root_dir,
build_env=build_env,
)
self._handle_build_requirements(
yield self._handle_build_requirements(
req,
RequirementType.BUILD_BACKEND,
build_backend_dependencies,
Expand All @@ -681,7 +692,7 @@ def _prepare_build_dependencies(
sdist_root_dir=sdist_root_dir,
build_env=build_env,
)
self._handle_build_requirements(
yield self._handle_build_requirements(
req,
RequirementType.BUILD_SDIST,
build_sdist_dependencies,
Expand All @@ -702,14 +713,12 @@ def _handle_build_requirements(
req: Requirement,
build_type: RequirementType,
build_dependencies: set[Requirement],
) -> None:
) -> typing.Generator[typing.Any, typing.Any, None]:
self.progressbar.update_total(len(build_dependencies))

for dep in self._sort_requirements(build_dependencies):
with req_ctxvar_context(dep):
# In test mode, bootstrap() catches and records failures internally.
# In normal mode, it raises immediately which we propagate.
self.bootstrap(req=dep, req_type=build_type)
yield self._bootstrap_gen(req=dep, req_type=build_type)
self.progressbar.update()

def _download_prebuilt(
Expand Down Expand Up @@ -896,7 +905,7 @@ def _build_from_source(
build_sdist_only: bool,
cached_wheel_filename: pathlib.Path | None,
unpacked_cached_wheel: pathlib.Path | None,
) -> SourceBuildResult:
) -> typing.Generator[typing.Any, typing.Any, SourceBuildResult]:
"""Build package from source.

Orchestrates download, preparation, build environment setup, and build.
Expand Down Expand Up @@ -939,9 +948,7 @@ def _build_from_source(
)

# Prepare build dependencies (always needed)
# Note: This may recursively call bootstrap() for build deps,
# which has its own error handling.
self._prepare_build_dependencies(
yield self._prepare_build_dependencies(
req=req,
resolved_version=resolved_version,
sdist_root_dir=sdist_root_dir,
Expand Down Expand Up @@ -1334,11 +1341,13 @@ def _get_version_from_package_metadata(
ctx=self.ctx,
parent_dir=source_dir.parent,
)
build_dependencies = self._prepare_build_dependencies(
req=req,
resolved_version=None,
sdist_root_dir=source_dir,
build_env=build_env,
build_dependencies = trampoline(
Comment thread
dhellmann marked this conversation as resolved.
self._prepare_build_dependencies(
req=req,
resolved_version=None,
sdist_root_dir=source_dir,
build_env=build_env,
)
)
build_env.install(build_dependencies)

Expand Down
32 changes: 22 additions & 10 deletions tests/test_bootstrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import json
import pathlib
import typing
from unittest.mock import Mock, patch

import pytest
Expand All @@ -8,6 +9,7 @@
from packaging.utils import canonicalize_name
from packaging.version import Version
from resolvelib.resolvers import ResolverException
from trampoline import trampoline

from fromager import bootstrapper, requirements_file
from fromager.context import WorkContext
Expand Down Expand Up @@ -270,21 +272,29 @@ def test_build_from_source_returns_dataclass(tmp_context: WorkContext) -> None:
mock_wheel = tmp_context.work_dir / "package-1.0.0-py3-none-any.whl"
expected_unpack_dir = mock_sdist_root.parent

def mock_prepare_gen(
*args: typing.Any, **kwargs: typing.Any
) -> typing.Generator[typing.Any, typing.Any, set[Requirement]]:
return set()
yield

with (
patch("fromager.sources.download_source", return_value=mock_source_file),
patch("fromager.sources.prepare_source", return_value=mock_sdist_root),
patch("fromager.sources.get_source_type", return_value=SourceType.SDIST),
patch.object(bt, "_prepare_build_dependencies"),
patch.object(bt, "_prepare_build_dependencies", side_effect=mock_prepare_gen),
patch.object(bt, "_build_wheel", return_value=(mock_wheel, None)),
):
result = bt._build_from_source(
req=Requirement("test-package"),
resolved_version=Version("1.0.0"),
source_url="https://pypi.org/simple/test-package",
req_type=requirements_file.RequirementType.TOP_LEVEL,
build_sdist_only=False,
cached_wheel_filename=None,
unpacked_cached_wheel=None,
result = trampoline(
bt._build_from_source(
req=Requirement("test-package"),
resolved_version=Version("1.0.0"),
source_url="https://pypi.org/simple/test-package",
req_type=requirements_file.RequirementType.TOP_LEVEL,
build_sdist_only=False,
cached_wheel_filename=None,
unpacked_cached_wheel=None,
)
)

# Verify return type is SourceBuildResult
Expand Down Expand Up @@ -323,12 +333,14 @@ def mock_bootstrap_impl(
source_url: str,
resolved_version: Version,
build_sdist_only: bool,
) -> None:
) -> typing.Generator[typing.Any, typing.Any, None]:
call_count["count"] += 1
if str(resolved_version) == "1.5":
raise ValueError("Simulated failure for version 1.5")
# For other versions, just mark as seen to avoid actual build
bt._mark_as_seen(req, resolved_version, build_sdist_only)
return
yield

with patch.object(bt, "_bootstrap_impl", side_effect=mock_bootstrap_impl):
# Mock _has_been_seen to return False so we attempt bootstrap
Expand Down
Loading