fix: mo.status.progress_bar correctly handles range step#9612
Closed
z3r0-815 wants to merge 2 commits into
Closed
Conversation
progress_bar(range(0, 10, 2)) was incrementing by 2 per iteration instead of by 1, causing current to exceed total. len(range) already correctly computes total with step accounted for, so the fix is to always increment by 1 per iteration — counting iterations, not range values. Fixes marimo-team#9575
|
All contributors have signed the CLA ✍️ ✅ |
for more information, see https://pre-commit.ci
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
No issues found across 2 files
Architecture diagram
sequenceDiagram
participant UserCode as User Code
participant PB as progress_bar()
participant Progress as _Progress
participant Output as Output System
participant Collection as Collection (range/list/etc.)
Note over UserCode,Collection: Runtime Execution Flow
UserCode->>PB: Call progress_bar(range(0, 10, 2))
PB->>PB: Detect collection type (range)
PB->>PB: Compute total = len(range(0, 10, 2)) → 5
PB->>PB: Set increment = 1 per iteration
PB->>Progress: Create ProgressBar instance (total=5)
Progress-->>UserCode: ProgressBar iterator
loop For each iteration (5 total)
UserCode->>Progress: Request next item
Progress->>Collection: Get next value
Collection-->>Progress: Yield item (e.g., 0, 2, 4, 6, 8)
Progress->>Progress: Increment current by 1
alt Progress enabled
Progress->>Output: update(current, total)
Output-->>Progress: Rendered
end
Progress-->>UserCode: Yield item
end
UserCode->>Progress: Signal end of iteration
Progress->>Progress: Mark complete (current == total)
Progress->>Output: Final progress display (5/5)
Author
|
I have read the CLA Document and I hereby sign the CLA |
Contributor
|
thank you. we already have a PR up for this #9582 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9575
mo.status.progress_bar(range(0, 10, 2))shows impossible progress states. The bug is in howprogress_barcalculates the increment per iteration when the range has a non-default step.Root Cause
progress_bar.__init__extractsself.stepfromrangeobjects:Then
__iter__and__aiter__useself.stepas the increment:While
len(range(0, 10, 2))correctly computestotal = 5, the increment of2per iteration over5iterations producescurrent = 10— double the actual count. The progress bar shows impossible states like 8/5, 10/5.Fix
self.stepfield entirely1per iterationA progress bar counts loop iterations, not range values.
range.stepdetermines what values are yielded, not how many times the loop body executes. Eachyield= 1 unit of progress, regardless of the range step. This is consistent with howtqdmbehaves — it counts iterations, not step increments.Tests Added
currentaftertest_progress_bar_range_with_steprange(0, 10, 2)[0, 2, 4, 6, 8](5)test_progress_bar_range_no_steprange(10)[0..9](10)test_progress_bar_range_custom_steprange(5, 20, 3)[5, 8, 11, 14, 17](5)test_progress_bar_range_negative_steprange(10, 0, -2)[10, 8, 6, 4, 2](5)All 17 tests pass (13 existing + 4 new).
Checklist
self.steplogic removed, increment changed to1ruff checkandruff formatpass cleanly