feat: Add :progress_bar_step config option - #441
Conversation
641d962 to
ea2cadc
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a :progress_bar_step configuration option to reduce CI log flooding during model downloads. Instead of updating the progress bar on every network chunk (which creates hundreds of lines in CI logs where escape codes don't work), progress updates can now be limited to percentage boundaries.
Key changes:
- Added
:progress_bar_stepconfig option (default:nilfor current behavior) - Modified download progress tracking to include
last_percentstate - Implemented stepped progress rendering logic to only update at configured intervals
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/bumblebee/utils.ex |
Added progress_bar_step/0 function to retrieve the config value with type spec and documentation |
lib/bumblebee/utils/http.ex |
Updated download state to track last_percent and added maybe_render_progress/2 function to conditionally render progress based on step configuration |
README.md |
Added documentation and examples for the new progress bar configuration options |
Comments suppressed due to low confidence (1)
lib/bumblebee/utils/http.ex:102
- When using a step-based update, the progress bar may not show 100% completion. For example, with
step = 10, if the last rendered update is at 92% (saylast_percent = 92because it jumped from 89%), then reaching 100% won't trigger a render since100 >= 92 + 10evaluates to100 >= 102, which is false.
Consider either: (1) adding a final progress bar render in this handler when step is configured to always show 100% completion, or (2) using the boundary-crossing logic suggested for line 114, which would naturally handle this case by checking if div(100, 10) > div(92, 10) (i.e., 10 > 9, which is true).
{:error, error} ->
:httpc.cancel_request(state.request_id, :bumblebee)
馃挕 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0a7cb52 to
7dd3c0e
Compare
7dd3c0e to
9af33ac
Compare
| When set, progress updates only when crossing step boundaries (e.g., 10%, 20%, ...). | ||
| Defaults to `nil`, which updates on every chunk. | ||
| """ | ||
| @spec progress_bar_step :: non_neg_integer() | nil |
There was a problem hiding this comment.
It may be reasonable to make the default 1 or 0.5 to limit the IO, it shouldn't make much difference UX wise. Arguably if we make it 1, we don't necessarily need it to be configurable, since that should be fine on CI, but either works for me.
There was a problem hiding this comment.
default is now 1
The progress bar updates on every network chunk during downloads. This floods CI logs because the escape codes for in-place updates don't work there, so each update ends up on a new line.
Added a
:progress_bar_stepconfig option that limits updates to percentage boundaries:Default is
nil(current behavior). Setting it to10gives you ~10 lines of output instead of hundreds.