docs(proposal): add iterative bootstrap proposal#1097
docs(proposal): add iterative bootstrap proposal#1097LalatenduMohanty merged 1 commit intopython-wheel-build:mainfrom
Conversation
|
Warning Rate limit exceeded
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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new proposal document is introduced that specifies converting the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ 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. Review rate limit: 0/1 reviews remaining, refill in 56 minutes and 31 seconds.Comment |
Propose converting the recursive bootstrapper to an iterative approach using an explicit work stack to eliminate recursion depth limits. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
44e8ad8 to
ec28ac2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/proposals/iterative-bootstrap.rst (1)
27-30: Clarify install dependency ordering.Lines 27-30 discuss build dependencies being pushed in reverse order (BUILD_SYSTEM, BUILD_BACKEND, BUILD_SDIST), but don't mention install dependencies. The EXTRACT_INSTALL_DEPS phase exists (line 24), so the proposal should clarify whether install dependencies also require reverse-order pushing or follow different rules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/iterative-bootstrap.rst` around lines 27 - 30, The text omits how install dependencies are ordered relative to build deps: clarify whether EXTRACT_INSTALL_DEPS pushes install deps onto the same LIFO stack in reverse order (so last install-dep pushed is processed first) or uses a different ordering; update the paragraph to explicitly state the rule for install dependencies (e.g., "install dependencies discovered in EXTRACT_INSTALL_DEPS are also pushed in reverse order onto the LIFO stack so each install dependency completes all phases before its parent proceeds" or describe the alternate behavior), and mention any interaction with BUILD_SYSTEM, BUILD_BACKEND, BUILD_SDIST to make the complete ordering semantics unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/proposals/iterative-bootstrap.rst`:
- Line 21: The START phase spec is missing two operations used by
_bootstrap_single_version: after adding to the dependency graph and performing
the seen-check (early exit), call _mark_as_seen() before processing dependencies
to prevent re-entrance, and create the _track_why() context manager around all
subsequent phases so dependency-chain context is preserved for error reporting;
update the START description to state: add to graph, check seen (early exit if
true), call _mark_as_seen(), and establish the _track_why context for subsequent
phases.
- Around line 38-42: Clarify and document the iterative bootstrap error-handling
contract: specify that the try-except around _bootstrap_impl (or its iterative
equivalent) must cover phases from BUILD_PACKAGE through COMPLETE (i.e., all
work items processed in the iterative loop), describe that on exception the work
stack must be unwound deterministically by invoking the existing cleanup routine
for each pending work item (or a new cleanup_work_stack(work_stack)) to leave no
partial state, and state explicit rules that after a BUILD_PACKAGE failure the
EXTRACT_INSTALL_DEPS and COMPLETE phases must be skipped for that package (but
cleanup/rollback actions for any already-started phases must still run); also
enumerate which mode flags (test_mode and multiple_versions) alter behavior
(test_mode should re-raise immediately, multiple_versions should record the
failure and continue other versions) so implementers can implement the try/catch
behavior correctly.
---
Nitpick comments:
In `@docs/proposals/iterative-bootstrap.rst`:
- Around line 27-30: The text omits how install dependencies are ordered
relative to build deps: clarify whether EXTRACT_INSTALL_DEPS pushes install deps
onto the same LIFO stack in reverse order (so last install-dep pushed is
processed first) or uses a different ordering; update the paragraph to
explicitly state the rule for install dependencies (e.g., "install dependencies
discovered in EXTRACT_INSTALL_DEPS are also pushed in reverse order onto the
LIFO stack so each install dependency completes all phases before its parent
proceeds" or describe the alternate behavior), and mention any interaction with
BUILD_SYSTEM, BUILD_BACKEND, BUILD_SDIST to make the complete ordering semantics
unambiguous.
🪄 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: 8a88279f-56c7-4de8-8424-5107cf57ada8
📒 Files selected for processing (1)
docs/proposals/iterative-bootstrap.rst
|
I will be proceeding with the implementation of this tomorrow based on the plan linked in PR description. This is the next thing for multiple version bootstrap feature. See #1068 |
Propose converting the recursive bootstrapper to an iterative approach using an explicit work stack to eliminate recursion depth limits.
This proposal is mainly aimed for documentation purposes and is completely inspired from a plan created by @dhellmann which can be found here : https://github.com/dhellmann/fromager/blob/explore-non-recursive-bootstrap/plan.md