refactor(bootstrapper): convert recursive bootstrap to iterative proessing by rd4398 · Pull Request #1116 · python-wheel-build/fromager

rd4398 · 2026-05-05T19:06:08Z

Replace the recursive DFS in Bootstrapper.bootstrap() with an explicit LIFO stack and a six-phase state machine (START, PREPARE_SOURCE, PREPARE_BUILD, BUILD, PROCESS_INSTALL_DEPS, COMPLETE). This eliminates Python stack overflow on deep/wide dependency graphs, especially with --multiple-versions enabled.

Key changes:

Add BootstrapPhase enum and WorkItem dataclass for phase-based processing
Add six phase handler methods that replace the monolithic _bootstrap_impl
Rewrite bootstrap() to use an iterative DFS loop with explicit work stack
Remove _bootstrap_single_version, _bootstrap_impl, _build_from_source
Preserve all three error handling modes (normal, test, multiple-versions)
Keep _prepare_build_dependencies for git URL resolution path only
Update tests to match new internal structure

coderabbitai · 2026-05-05T19:06:21Z

Warning

Rate limit exceeded

@rd4398 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 45 minutes and 25 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 94d25d4e-29e8-495a-926a-f91f60e95656

📥 Commits

Reviewing files that changed from the base of the PR and between 854abb9 and b8e3f29.

📒 Files selected for processing (2)

src/fromager/bootstrapper.py
tests/test_bootstrapper.py

📝 Walkthrough

Walkthrough

The bootstrap() method was rewritten to use an explicit iterative LIFO loop over WorkItem objects carrying phase state, replacing the prior recursive single-version flow. Two new types (BootstrapPhase enum and WorkItem dataclass) model the end-to-end dependency preparation lifecycle. Six phase handlers (_phase_start, _phase_prepare_source, _phase_prepare_build, _phase_build, _phase_process_install_deps, _phase_complete) replace prior monolithic logic, with centralized error handling via _handle_phase_error(). The _handle_build_requirements() method was adjusted to preserve self.why stack integrity during nested bootstraps. Tests were simplified to validate the new phase-build handler and reworked to test error handling at the dispatch level.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: converting recursive bootstrap to iterative processing using a state machine approach.
Description check	✅ Passed	The description is well-detailed and directly explains the refactor's purpose, key changes, and benefits related to the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

rd4398 · 2026-05-05T19:09:13Z

We have to choose between #1109 and this PR to proceed forward. Please take a look and let me know what you think.

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/fromager/bootstrapper.py`:
- Around line 1489-1494: The code currently unions item.build_backend_deps and
item.build_sdist_deps which relabels BUILD_SDIST edges as BUILD_BACKEND and
loses the duplicate-edge semantics; change the logic in the place that builds
dep_items (call _create_dep_work_items) to handle the two dependency sets
separately instead of doing item.build_backend_deps | item.build_sdist_deps:
call _create_dep_work_items(item.build_backend_deps,
RequirementType.BUILD_BACKEND, item.req, item.resolved_version) and separately
_create_dep_work_items(item.build_sdist_deps, RequirementType.BUILD_SDIST,
item.req, item.resolved_version), then combine/extend their results so both
typed edges are preserved (so self.why, _add_to_graph and deduping behavior
remain correct).
- Around line 1667-1677: When removing a failed version in the multiple_versions
branch, also remove the "seen" marker that _phase_start() previously set so
future traversals can retry that version; specifically after calling
self.ctx.dependency_graph.remove_dependency(...) and before
self.ctx.write_to_graph_to_file(), drop the entry from whatever in-memory
seen-tracking structure used by _phase_start() (e.g.
self._seen.discard((pkg_name, str(item.resolved_version))) or call the
corresponding clear/unmark method), and ensure _failed_versions append and
logging remain unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cacc4cdb-4289-4fbf-8845-dab740b0d233

📥 Commits

Reviewing files that changed from the base of the PR and between 1429c1a and 854abb9.

📒 Files selected for processing (2)

src/fromager/bootstrapper.py
tests/test_bootstrapper.py

…essing Replace the recursive DFS in Bootstrapper.bootstrap() with an explicit LIFO stack and a six-phase state machine (START, PREPARE_SOURCE, PREPARE_BUILD, BUILD, PROCESS_INSTALL_DEPS, COMPLETE). This eliminates Python stack overflow on deep/wide dependency graphs, especially with --multiple-versions enabled. Key changes: - Add BootstrapPhase enum and WorkItem dataclass for phase-based processing - Add six phase handler methods that replace the monolithic _bootstrap_impl - Rewrite bootstrap() to use an iterative DFS loop with explicit work stack - Remove _bootstrap_single_version, _bootstrap_impl, _build_from_source - Preserve all three error handling modes (normal, test, multiple-versions) - Keep _prepare_build_dependencies for git URL resolution path only - Update tests to match new internal structure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>

dhellmann · 2026-05-05T20:30:26Z

+    populated across phases as processing advances.
+    """
+
+    # Identity (set at creation)


How important is it that these "identity" values don't change? I wonder if we want to put them in their own data class so we can mark it as frozen, for example. Maybe that's overkill?

dhellmann · 2026-05-05T20:35:06Z

+        # (the loop modifies self.why for each work item)
+        saved_why = list(self.why)
+
+        # Create initial work items (reversed so first version is on top of stack)


What does "first" mean in this context? Lowest version number?

dhellmann · 2026-05-05T20:36:16Z

+        # Main iterative DFS loop
+        while stack:
+            item = stack.pop()
+            self.why = list(item.why_snapshot)


I wonder if instead of resetting self.why we could use item.why_snapshot in all of those places? Do we have the item value everywhere we would need to do that?

dhellmann · 2026-05-05T20:37:05Z

+                    if item.phase == BootstrapPhase.START:
+                        # START phase runs outside _track_why to match
+                        # original behavior where seen-check precedes
+                        # pushing onto the why stack.


What would be the effect of eliminating that special case?

dhellmann · 2026-05-05T20:39:27Z

+            if not new_items:
+                self.progressbar.update()
+
+            # Push items so first in list ends up on top (processed first)


I think we need to reverse the list here because we're not treating the list in the phase functions as a stack. I would have expected the phase to return something like [item] + new_items and that resulting list would then need to have new_items processed before trying item again. That way extend() here just adds to the stack in the same natural order.

dhellmann · 2026-05-05T20:41:58Z

+
+        Called outside _track_why context to match original behavior
+        where graph addition and seen-check happen before pushing
+        the current item onto the why stack.


It would help to explicitly explain what the return list is for each phase function. It's here in this paragraph, but calling it out as something like this would help make the flow more obvious:

Returns the current item to push it back onto the stack for the prepare-source phase.

dhellmann · 2026-05-05T20:43:15Z

+        )
+
+        item.phase = BootstrapPhase.PREPARE_BUILD
+        return dep_items + [item]


Treat the return value as a stack. We want to process dep_items before looking at item again.

Suggested change

return dep_items + [item]

return [item] + dep_items

dhellmann · 2026-05-05T20:48:49Z

+        return []
+
+    def _dispatch_phase(self, item: WorkItem) -> list[WorkItem]:
+        """Route a work item to the appropriate phase handler."""


Do we log the phase anywhere?

dhellmann · 2026-05-05T20:55:34Z

+
+        # Create initial work items (reversed so first version is on top of stack)
+        stack: list[WorkItem] = []
+        for source_url, resolved_version in reversed(resolved_versions):


What do you think about making resolving the versions a phase, too?

dhellmann · 2026-05-05T20:58:07Z

+        items: list[WorkItem] = []
+        for dep in self._sort_requirements(deps):
+            try:
+                resolved = self.resolve_versions(


Making the version resolution a phase would mean we would only have this logic in 1 place.

rd4398 requested a review from a team as a code owner May 5, 2026 19:06

rd4398 requested review from LalatenduMohanty and dhellmann May 5, 2026 19:06

mergify Bot added the ci label May 5, 2026

rd4398 mentioned this pull request May 5, 2026

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

Open

coderabbitai Bot reviewed May 5, 2026

View reviewed changes

Comment thread src/fromager/bootstrapper.py Outdated

Comment thread src/fromager/bootstrapper.py

rd4398 force-pushed the iterative-bootstrap-with-stages branch from 88e0673 to b8e3f29 Compare May 5, 2026 19:20

dhellmann reviewed May 5, 2026

View reviewed changes

Conversation

rd4398 commented May 5, 2026

Uh oh!

coderabbitai Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Estimated code review effort

Uh oh!

rd4398 commented May 5, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented May 5, 2026 •

edited

Loading