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 tick to MultiProgress to complement join #132

Closed
wants to merge 2 commits into from

Conversation

mibac138
Copy link
Contributor

@mibac138 mibac138 commented Nov 8, 2019

  • Changed joining field of MultiProgress from AtomicBool to Mutex.
    Because the internal join_impl can now be called multiple times
    I figured it's better to use a Mutex as it automatically unlocks
    preventing panics "bricking" the progress bar.
  • Because MultiProgress can contain multiple child progress bars I feel
    like the meaning of a single tick isn't really clear. Just a single
    state update (self.rx.recv()) seems too small. Because of that I
    allow the user to configure how long do they want a tick to last
    adding greater flexibility.
  • Made is_done public to let the user know whether they still need
    to invoke tick

Fixes #33, seems like fixes #125

@dan-da
Copy link

dan-da commented Mar 17, 2020

I am in need of a fix for #33 asap. Will this make it into a new release soon?

@mibac138
Copy link
Contributor Author

@dan-da If you want to you can modify your Cargo.toml to use my PR instead of the published version. To do this you need to replace indicatif = "0.14.0" with indicatif = { git="https://github.com/mibac138/indicatif", branch="mpb-tick" } and it should work. I'd appreciate any feedback you might have if you do use this code.

@dan-da
Copy link

dan-da commented Mar 17, 2020

@mibac138 hey thanks. I just tried with your branch, but nothing displays, same as with 0.14.0. I am just creating a MultiProgress with 3 ProgressBar+style and then calling set_position on each, in a single thread Any chance you can provide a working single-threaded example?

Also, I tried to call MultiProgress::tick() just to see if it helps, but it requires a progress::TickTimeLimit and indacitif::progress is private.

btw, my particular use-case is like the download example, except I want to show:
line1: spinner + filename (present download)
line2: file download progress bar
line3: Entire transfer (multi-file) progress bar

@mibac138
Copy link
Contributor Author

@dan-da I'm not sure what is it that doesn't work for you but I've written a simple example below:

use indicatif::{MultiProgress, ProgressBar, TickTimeLimit};
use rand::Rng;

fn main() {
    let mpb = MultiProgress::new();
    let pb1 = mpb.add(ProgressBar::new(30));
    let pb2 = mpb.add(ProgressBar::new(30));

    let mut rng = rand::thread_rng();
    for _ in 0..10 {
        thread::sleep(Duration::from_secs(1));
        pb1.set_position(rng.gen_range(0, 30));
        pb2.set_position(rng.gen_range(0, 30));
        mpb.tick(TickTimeLimit::Indefinite);
    }
}

@dan-da
Copy link

dan-da commented Mar 17, 2020

It is working great now. Thx for the example. I was missing the tick() call.

@dan-da
Copy link

dan-da commented Mar 18, 2020

hmm, I do have one display prob. I'm not sure if it's a problem with the lib, or my usage of it. So I have two progress bars with {elapsed_precise} in the template, and one bar (A) is updated more frequently with a new position (via set_position()) than the other (B). After each update, which occurs every 500 milliseconds, tick() is called on the MultiProgress that contains both, with a 50ms Duration. The issue is that the elapsed seconds in A sometimes update before B. Usually they are in sync, each second, but not always. I'm not sure if TickTimeLimit::Timeout is supposed to work like ProgressBar::enable_steady_tick()? I just want the elapsed time to update in unison for both bars each time.

If above is not clear, here's a simplified version of my update func, which is invoked from a long-running process.

// pass bars vec in separately because MultiProgress doesn't seem to have a 
// getter for the ProgressBar that were added to it.  Did I miss something?
fn update_progress_bars(m: &MultiProgress, bars: &[ProgressBar], status: &Status) {

    bars[1].set_position(status.file_bytes_written);            // value changes more often
    bars[2].set_position(status.transfer_bytes_written);     // value changes less often

    // trying to get elapsed time clocks updating secs in unison.
    m.tick(TickTimeLimit::Timeout(Duration::from_millis(50)) );  

    thread::sleep(Duration::from_millis(500));   // simulate lengthy work done for 1/2 second.
}

edit: I did a bit more testing, setting the sleep to 2000, and two things become clear:

  1. Elapsed time for both bars take at least 2 secs to update, so its not working like ProgressBar::enable_steady_tick(). Maybe I should call that on each bar during setup? Or do we need a MultiProgress::enabley_steady_tick()?
  2. B's elapsed time display only updates/paints if the value in status.transfer_bytes_written has actually changed, even though set_position() and tick() were both called.

@mibac138
Copy link
Contributor Author

@dan-da I believe this might be due to the refresh rate of your draw target. Try changing how you created MultiProgress from presumably MultiProgress::new() to MultiProgress::with_draw_target(ProgressDrawTarget::stderr_nohz()). This issue is caused by how the MultiProgress works compared to ProgressBar. Regular progress bars when updating have immediate access to the latest progress state, so when they are drawn everything is based on the latest state. However, MultiProgress internally uses mpsc::channel to communicate with progress bars and it's implemented so that every update of a child progress bar is a single draw which makes it so that if there are two progress bars which need drawing there will be two separate draw calls and the default draw target will throttle those draws.
This also means there isn't a direct replacement for enable_steady_tick in MultiProgress. TickTimeLimit::Timeout and Deadline boil down to the same thing but are divided to improve ergonomics similar to how mpsc::channel has both recv_timeout and recv_deadline.

@dan-da
Copy link

dan-da commented Mar 24, 2020

hi, yeah I've tried with stderr_nohz(), and I also tried creating a thread that calls MultiProgress::tick() every 10ms. Neither approach causes the elapsed_precise clock to tick each second as desired.

Here is a test case that demonstrates the issue, with a for loop that is updating the progress bars every 2 seconds, and the clock also updates every 2 secs.

use indicatif::{MultiProgress, ProgressBar, TickTimeLimit, ProgressStyle, ProgressDrawTarget};
use rand::Rng;
use std::thread;
use std::time::Duration;



fn main() {
    let sty = ProgressStyle::default_bar()
        .template("{spinner:.green} [{elapsed_precise}] [{bar:40.cyan/blue}] {bytes}/{total_bytes} ({bytes_per_sec}, {eta})  File")
        .progress_chars("##-");

    let mpb = MultiProgress::with_draw_target(ProgressDrawTarget::stderr_nohz());
    let pb1 = mpb.add(ProgressBar::new(30));
    let pb2 = mpb.add(ProgressBar::new(30));

    pb1.set_style(sty.clone());
    pb2.set_style(sty);

    // To use thread, uncomment this and comment mbp.tick() at bottom.
/*    
    let _ = thread::spawn(move || {
        loop {
            mpb.tick(TickTimeLimit::Indefinite);
            thread::sleep(Duration::from_millis(10));
        }
    });
*/

    let mut rng = rand::thread_rng();
    for _ in 0..50 {
        thread::sleep(Duration::from_millis(2000));
        pb1.set_position(rng.gen_range(0, 30));
        pb2.set_position(rng.gen_range(0, 30));
        
        mpb.tick(TickTimeLimit::Indefinite);
    }
}

* Changed `joining` field of `MultiProgress` from `AtomicBool` to `Mutex`.
  Because the internal `join_impl` can now be called multiple times
  I figured it's better to use a Mutex as it automatically unlocks
  preventing panics "bricking" the progress bar.
* Because MultiProgress can contain multiple child progress bars I feel
  like the meaning of a single tick isn't really clear. Just a single
  state update (`self.rx.recv()`) seems too small. Because of that I
  allow the user to configure how long do they want a tick to last
  adding greater flexibility.
* Made `is_done` public to let the user know whether they still need
  to invoke `tick`
shenek added a commit to shenek/wait-for-them that referenced this pull request May 29, 2020
Indicatif MultiProgress doesn't work well in the static context
This is basically a workaround for that.
A regular thread is spawned to update the progress in the background.

Note that the code might be refactored to the way it was before.
Once console-rs/indicatif#132 is merged.
shenek added a commit to shenek/wait-for-them that referenced this pull request May 29, 2020
Indicatif MultiProgress doesn't work well in the static context
This is basically a workaround for that.
A regular thread is spawned to update the progress in the background.

Note that the code might be refactored to the way it was before.
Once console-rs/indicatif#132 is merged.
shenek added a commit to shenek/wait-for-them that referenced this pull request May 29, 2020
Indicatif MultiProgress doesn't work well in the static context
This is basically a workaround for that.
A regular thread is spawned to update the progress in the background.

Note that the code might be refactored to the way it was before.
Once console-rs/indicatif#132 is merged.
@mitsuhiko mitsuhiko closed this Jun 14, 2020
@dan-da
Copy link

dan-da commented Jun 14, 2020

Closed without comment on the test case I posted?

@mitsuhiko mitsuhiko reopened this Jun 25, 2020
@mitsuhiko mitsuhiko changed the base branch from master to main June 25, 2020 11:14
@etrombly
Copy link

etrombly commented Sep 9, 2020

This pull is working for me for single threaded MultiProgress. Is there anything blocking it being merged? elapsed_precise is working https://github.com/etrombly/dropcutter

@mibac138 mibac138 mentioned this pull request Apr 16, 2021
@djc
Copy link
Collaborator

djc commented Feb 22, 2022

I don't think this is necessary with current main, but if still is feel free to open a new issue/PR.

@djc djc closed this Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MultiProgress should support tokio (async) Single-threaded MultiProgress
5 participants