Skip to content

Refactor(bootstrapper): add force_prebuilt flag, remove _handle_test_mode_failure #894

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
LalatenduMohanty wants to merge 1 commit into
base: main
Choose a base branch
from
+304 −163
Open

Refactor(bootstrapper): add force_prebuilt flag, remove _handle_test_mode_failure #894

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
293 changes: 133 additions & 160 deletions src/fromager/bootstrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,35 +294,76 @@ def bootstrap(self, req: Requirement, req_type: RequirementType) -> None:
self._bootstrap_impl(
req, req_type, source_url, resolved_version, build_sdist_only
)
except Exception as err:
except Exception as build_error:
if not self.test_mode:
raise

Copy link
Copy Markdown
Member

@dhellmann dhellmann Mar 27, 2026

Choose a reason for hiding this comment

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

You could add a check here to avoid trying again if the package is already marked as pre-built. I don't know how often that would come up, but in a deep dependency graph it would short-cut an expensive and useless operation.

# Test mode: record the build failure unconditionally
self._record_test_mode_failure(
req, str(resolved_version), err, "bootstrap"
req, str(resolved_version), build_error, "bootstrap"
)

# Attempt pre-built fallback
logger.warning(
"test mode: build failed for %s==%s, attempting pre-built fallback: %s",
req.name,
resolved_version,
build_error,
)
try:
actual_version = self._bootstrap_impl(
req,
req_type,
source_url,
resolved_version,
build_sdist_only,
force_prebuilt=True,
)
logger.info(
"test mode: successfully used pre-built wheel for %s==%s",
req.name,
actual_version,
)
except Exception as fallback_error:
logger.error(
"test mode: pre-built fallback also failed for %s: %s",
req.name,
fallback_error,
exc_info=True,
)
# Build failure already recorded above. Re-raise original
# error so callers know the package wasn't installed.
raise build_error from fallback_error

def _bootstrap_impl(
self,
req: Requirement,
req_type: RequirementType,
source_url: str,
resolved_version: Version,
build_sdist_only: bool,
) -> None:
force_prebuilt: bool = False,
) -> Version:
"""Internal implementation - performs the actual bootstrap work.

Called by bootstrap() after setup, validation, and seen-checking.

Args:
req: The requirement to bootstrap.
req_type: The type of requirement.
source_url: The resolved source URL.
source_url: The resolved source URL (sdist or wheel URL).
resolved_version: The resolved version.
build_sdist_only: Whether to build only sdist (no wheel).
force_prebuilt: If True, resolve a pre-built wheel URL and skip
source build. Used for test-mode fallback after build failure.

Returns:
The version that was actually installed. This may differ from
resolved_version when force_prebuilt=True and the prebuilt
resolver returns a different version.

Error Handling:
Fatal errors (source build, prebuilt download) raise exceptions
for bootstrap() to catch and record.
Fatal errors (source build, prebuilt download) raise exceptions.

Non-fatal errors (post-hook, dependency extraction) are recorded
locally and processing continues. These are recorded here because
Expand All @@ -339,12 +380,35 @@ def _bootstrap_impl(
cached_wheel_filename: pathlib.Path | None = None
unpacked_cached_wheel: pathlib.Path | None = None

if pbi.pre_built:
if pbi.pre_built or force_prebuilt:
if force_prebuilt:
# Resolve prebuilt wheel URL (source_url is the sdist URL)
parent_req = self.why[-1][1] if self.why else None
wheel_url, fallback_version = self._resolver.resolve(
req=req,
req_type=req_type,
parent_req=parent_req,
pre_built=True,
)
Comment on lines +383 to +392
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Pass the actual parent into the prebuilt resolver.

By the time this branch runs, _track_why() has already pushed req, so self.why[-1][1] is the package being retried, not its parent. Top-level fallbacks therefore resolve with parent_req=req, and transitive fallbacks resolve with the wrong parent.

🔧 Minimal fix 
-                parent_req = self.why[-1][1] if self.why else None
+                parent_req = self.why[-2][1] if len(self.why) > 1 else None
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/fromager/bootstrapper.py` around lines 383 - 392, The current call to
self._resolver.resolve in the pbi.pre_built / force_prebuilt branch uses
self.why[-1][1] as parent_req but _track_why() has already pushed the current
req, so that yields the wrong parent; instead determine the real parent
before/independent of the push and pass it to resolve (e.g. use the previous
entry self.why[-2][1] when self.why has at least two entries, otherwise None) so
that self._resolver.resolve(req=..., req_type=req_type,
parent_req=actual_parent, pre_built=True) receives the correct parent package.

if fallback_version != resolved_version:
logger.warning(
"test mode: version mismatch for %s"
" - requested %s, fallback %s",
req.name,
resolved_version,
fallback_version,
)
resolved_version = fallback_version
# Update source_url so _add_to_build_order records
# the wheel URL, not the original sdist URL.
source_url = wheel_url
else:
wheel_url = source_url
wheel_filename, unpack_dir = self._download_prebuilt(
req=req,
req_type=req_type,
resolved_version=resolved_version,
wheel_url=source_url,
wheel_url=wheel_url,
)
build_result = SourceBuildResult(
wheel_filename=wheel_filename,
Expand All @@ -360,12 +424,11 @@ def _bootstrap_impl(
req, resolved_version
)

# Build from source (handles test-mode fallback internally)
# Build from source
build_result = self._build_from_source(
req=req,
resolved_version=resolved_version,
source_url=source_url,
req_type=req_type,
build_sdist_only=build_sdist_only,
cached_wheel_filename=cached_wheel_filename,
unpacked_cached_wheel=unpacked_cached_wheel,
Expand Down Expand Up @@ -428,14 +491,19 @@ 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)
try:
self.bootstrap(req=dep, req_type=RequirementType.INSTALL)
except Exception:
if not self.test_mode:
raise
# Failure already recorded in failed_packages; continue.
self.progressbar.update()
Comment on lines +494 to 500
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Only suppress child bootstrap errors after confirming they were recorded.

These blocks assume every test-mode exception from bootstrap() has already appended to failed_packages, but that is not guaranteed for failures before _record_test_mode_failure() runs, such as self.ctx.write_to_graph_to_file() inside _add_to_graph() for transitive dependencies. Swallowing those exceptions here can let finalize() report success even though dependencies were skipped. The same guard is needed in the top-level loop in src/fromager/commands/bootstrap.py.

🔧 Suggested guard 
+                failures_before = len(self.failed_packages)
                 try:
                     self.bootstrap(req=dep, req_type=RequirementType.INSTALL)
                 except Exception:
-                    if not self.test_mode:
+                    if (
+                        not self.test_mode
+                        or len(self.failed_packages) == failures_before
+                    ):
                         raise
                     # Failure already recorded in failed_packages; continue.

Also applies to: 686-692

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/fromager/bootstrapper.py` around lines 494 - 500, The try/except that
calls self.bootstrap(req=dep, req_type=RequirementType.INSTALL) should only
swallow exceptions in test_mode if the failure has been recorded to
failed_packages; update the except block to verify the exception was logged
(e.g., check failed_packages or that _record_test_mode_failure was called) and
re-raise if not, to avoid hiding failures from finalize(); also apply the same
guard to the analogous top-level loop handling in the bootstrap command (ensure
failures like those raised in _add_to_graph / ctx.write_to_graph_to_file are
recorded before suppressing).


# Clean up build directories
self.ctx.clean_build_dirs(build_result.sdist_root_dir, build_result.build_env)

return resolved_version

@contextlib.contextmanager
def _track_why(
self,
Expand Down Expand Up @@ -615,9 +683,12 @@ def _handle_build_requirements(

for dep in self._sort_requirements(build_dependencies):
Copy link
Copy Markdown
Member

@dhellmann dhellmann Mar 27, 2026

Choose a reason for hiding this comment

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

We have several loops doing this same thing now. Maybe a future PR could refactor those into a method to bootstrap an iterable of requirements. Don't update this one with that change, though.

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)
try:
self.bootstrap(req=dep, req_type=build_type)
except Exception:
if not self.test_mode:
raise
# Failure already recorded in failed_packages; continue.
self.progressbar.update()

def _download_prebuilt(
Expand Down Expand Up @@ -800,174 +871,76 @@ def _build_from_source(
req: Requirement,
resolved_version: Version,
source_url: str,
req_type: RequirementType,
build_sdist_only: bool,
cached_wheel_filename: pathlib.Path | None,
unpacked_cached_wheel: pathlib.Path | None,
) -> SourceBuildResult:
"""Build package from source.

Orchestrates download, preparation, build environment setup, and build.
In test mode, attempts pre-built fallback on failure.

Raises:
Exception: In normal mode, if build fails.
In test mode, only if build fails AND fallback also fails.
Exception: If any step (download, prepare, build) fails.
Callers are responsible for fallback handling.
"""
try:
# Download and prepare source (if no cached wheel)
if not unpacked_cached_wheel:
logger.debug("no cached wheel, downloading sources")
source_filename = self._download_source(
req=req,
resolved_version=resolved_version,
source_url=source_url,
)
sdist_root_dir = self._prepare_source(
req=req,
resolved_version=resolved_version,
source_filename=source_filename,
)
else:
logger.debug(f"have cached wheel in {unpacked_cached_wheel}")
sdist_root_dir = unpacked_cached_wheel / unpacked_cached_wheel.stem

assert sdist_root_dir is not None

if sdist_root_dir.parent.parent != self.ctx.work_dir:
raise ValueError(
f"'{sdist_root_dir}/../..' should be {self.ctx.work_dir}"
)
unpack_dir = sdist_root_dir.parent

build_env = self._create_build_env(
req=req,
resolved_version=resolved_version,
parent_dir=sdist_root_dir.parent,
)

# Prepare build dependencies (always needed)
# Note: This may recursively call bootstrap() for build deps,
# which has its own error handling.
self._prepare_build_dependencies(
# Download and prepare source (if no cached wheel)
if not unpacked_cached_wheel:
logger.debug("no cached wheel, downloading sources")
source_filename = self._download_source(
req=req,
resolved_version=resolved_version,
sdist_root_dir=sdist_root_dir,
build_env=build_env,
)

# Build wheel or sdist
wheel_filename, sdist_filename = self._do_build(
req=req,
resolved_version=resolved_version,
sdist_root_dir=sdist_root_dir,
build_env=build_env,
build_sdist_only=build_sdist_only,
cached_wheel_filename=cached_wheel_filename,
)

source_type = sources.get_source_type(self.ctx, req)

return SourceBuildResult(
wheel_filename=wheel_filename,
sdist_filename=sdist_filename,
unpack_dir=unpack_dir,
sdist_root_dir=sdist_root_dir,
build_env=build_env,
source_type=source_type,
source_url=source_url,
)

except Exception as build_error:
if not self.test_mode:
raise

# Test mode: attempt pre-built fallback
fallback_result = self._handle_test_mode_failure(
sdist_root_dir = self._prepare_source(
req=req,
resolved_version=resolved_version,
req_type=req_type,
build_error=build_error,
source_filename=source_filename,
)
if fallback_result is None:
# Fallback failed, re-raise for bootstrap() to catch
raise
else:
logger.debug(f"have cached wheel in {unpacked_cached_wheel}")
sdist_root_dir = unpacked_cached_wheel / unpacked_cached_wheel.stem

return fallback_result
assert sdist_root_dir is not None

def _handle_test_mode_failure(
self,
req: Requirement,
resolved_version: Version,
req_type: RequirementType,
build_error: Exception,
) -> SourceBuildResult | None:
"""Handle build failure in test mode by attempting pre-built fallback.

Args:
req: The requirement that failed to build.
resolved_version: The version that was attempted.
req_type: The type of requirement (for fallback resolution).
build_error: The original exception from the build attempt.
if sdist_root_dir.parent.parent != self.ctx.work_dir:
raise ValueError(f"'{sdist_root_dir}/../..' should be {self.ctx.work_dir}")
unpack_dir = sdist_root_dir.parent

Returns:
SourceBuildResult if fallback succeeded, None if fallback also failed.
"""
logger.warning(
"test mode: build failed for %s==%s, attempting pre-built fallback: %s",
req.name,
resolved_version,
build_error,
build_env = self._create_build_env(
req=req,
resolved_version=resolved_version,
parent_dir=sdist_root_dir.parent,
)

try:
parent_req = self.why[-1][1] if self.why else None
wheel_url, fallback_version = self._resolver.resolve(
req=req,
req_type=req_type,
parent_req=parent_req,
pre_built=True, # Force prebuilt for test mode fallback
)

if fallback_version != resolved_version:
logger.warning(
"test mode: version mismatch for %s - requested %s, fallback %s",
req.name,
resolved_version,
fallback_version,
)

wheel_filename, unpack_dir = self._download_prebuilt(
req=req,
req_type=req_type,
resolved_version=fallback_version,
wheel_url=wheel_url,
)
# Prepare build dependencies. Recursive bootstrap() calls handle
# their own error recovery in test mode.
self._prepare_build_dependencies(
req=req,
resolved_version=resolved_version,
sdist_root_dir=sdist_root_dir,
build_env=build_env,
)

logger.info(
"test mode: successfully used pre-built wheel for %s==%s",
req.name,
fallback_version,
)
# Package succeeded via fallback - no failure to record
# Build wheel or sdist
wheel_filename, sdist_filename = self._do_build(
req=req,
resolved_version=resolved_version,
sdist_root_dir=sdist_root_dir,
build_env=build_env,
build_sdist_only=build_sdist_only,
cached_wheel_filename=cached_wheel_filename,
)

return SourceBuildResult(
wheel_filename=wheel_filename,
sdist_filename=None,
unpack_dir=unpack_dir,
sdist_root_dir=None,
build_env=None,
source_type=SourceType.PREBUILT,
)
source_type = sources.get_source_type(self.ctx, req)

except Exception as fallback_error:
logger.error(
"test mode: pre-built fallback also failed for %s: %s",
req.name,
fallback_error,
exc_info=True,
)
# Return None to signal failure; bootstrap() will record via re-raised exception
return None
return SourceBuildResult(
wheel_filename=wheel_filename,
sdist_filename=sdist_filename,
unpack_dir=unpack_dir,
sdist_root_dir=sdist_root_dir,
build_env=build_env,
source_type=source_type,
)

def _look_for_existing_wheel(
self,
Expand Down
Loading
Loading