Skip to content
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.

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

Add Progress Bar For Large Streamed Downloads #6096

Merged
merged 10 commits into from
Jun 14, 2023

Conversation

indiv0
Copy link
Contributor

@indiv0 indiv0 commented Mar 28, 2023

Pull Request Description

This PR adds a progress bar to streamed downloads.

The build script displays a progress bar when downloading from a remote URL in non-streaming mode. It did not display a progress bar when downloading from a URL in streaming mode. Without a progress bar, large downloads in the build script make it appear as through the build script is stuck.

For example, when running with a remote backend source, the project-manager-bundle must be downloaded. This bundle is currently >1GB in size, so it takes several minutes to download. To avoid the appearance of the build script being stuck, we add a progress bar.

Screen.Recording.2023-03-28.at.09.59.22.mov

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@indiv0 indiv0 added the -build-script Category: build script label Mar 28, 2023
@indiv0 indiv0 requested a review from wdanilo March 28, 2023 07:01
@indiv0 indiv0 requested a review from mwu-tow as a code owner March 28, 2023 07:01
@indiv0 indiv0 added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 28, 2023
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

I always wanted it so badly! That's amazing!

@indiv0
Copy link
Contributor Author

indiv0 commented Mar 30, 2023

@PabloBuchu could I get a QA on this PR please?

@PabloBuchu
Copy link
Contributor

PabloBuchu commented Mar 31, 2023

QA 🔴 Everything works fine although I couldn't make the loading bar to appear @indiv0 🤔

@indiv0
Copy link
Contributor Author

indiv0 commented Apr 1, 2023

@PabloBuchu I've fixed the issue you reported on discord RE: the progress bar clobbering logging output. Would you mind QA'ing again?

That is, it should no longer do this:

image

@enso-bot
Copy link

enso-bot bot commented Apr 3, 2023

Nikita Pekin reports a new STANDUP for the provided date (2023-03-31):

Progress: Trying to resolve issues with logging not working correctly for large file downloads in build-cli. It should be finished by 2023-04-03.

@indiv0
Copy link
Contributor Author

indiv0 commented Apr 17, 2023

@mwu-tow would it be possible to get a QA on this? This touches the build process so it needs to be a cross-platform QA :)

(Pawel doesn't have a Windows or Linux machine)

@mwu-tow
Copy link
Contributor

mwu-tow commented Apr 17, 2023

@indiv0 I will do.

@mwu-tow
Copy link
Contributor

mwu-tow commented Apr 18, 2023

@indiv0

I did testing on Windows. Some observations:

  1. Sometimes a leftover progress bar still remains in the terminal:
    obraz
    It seems to happen when the Command is printed, likely because its debug representation contains a newline character.
  2. How does this IndicatifWriter approach compares with the https://docs.rs/indicatif/0.17.3/indicatif/struct.MultiProgress.html#method.println? It was my earlier, unfinished attempt at achieving this.
  3. Does this mean that we can write only to stderr without breaking progress bars?

Minor observations (feel free to leave them for later):

  • Having some kind of information about what the progress bar is about would improve clarity
  • Using pretty-printed size units rather than just total byte number would be more readable.
Manages multiple progress bars from different threads

@indiv0
Copy link
Contributor Author

indiv0 commented Jun 13, 2023

@mwu-tow

  1. ! Looks like this was potentially fixed in Handle newline in msg and empty msg console-rs/indicatif#540, would you mind re-testing?
  2. My understanding is that the IndicatifWriter approach is more general, since with the println approach there's a potential for output to overwrite the progress bar if there's multi-line output being written. With the IndicatifWriter approach, this shouldn't happen since the progress bar is removed before writing logs and re-drawn from scratch after. I may be wrong on this.
  3. I'm not entirely sure, but I didn't see any issues with that in my testing.
  4. I tried to add information to the progress bar, but that did break rendering, depending on how much info was provided so I decided to leave it out.
  5. Pretty-printed size units would be helpful but I'm not sure how best to do that. Just manually convert the bytes to MB and print that?

@indiv0 indiv0 requested a review from mwu-tow June 13, 2023 09:59
@indiv0 indiv0 merged commit 1fdad39 into develop Jun 14, 2023
@indiv0 indiv0 deleted the wip/np/stream-response-progress-bar branch June 14, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-build-script Category: build script CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants