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

Slower refresh than 1hz? #452

Open
mpetri opened this issue Jul 30, 2022 · 12 comments · May be fixed by #487
Open

Slower refresh than 1hz? #452

mpetri opened this issue Jul 30, 2022 · 12 comments · May be fixed by #487

Comments

@mpetri
Copy link

mpetri commented Jul 30, 2022

Previously I could refresh progress once every 5 seconds for example. Now the lowest refresh rate seems to be once per second with https://docs.rs/indicatif/latest/indicatif/struct.ProgressDrawTarget.html#method.stdout_with_hz and stdout_with_hz(1).

Is there a way to achieve this currently?

@djc
Copy link
Collaborator

djc commented Jul 30, 2022

You can't. Why would you want to?

@mpetri
Copy link
Author

mpetri commented Jul 30, 2022

For progress bars with many items that run over many hours I find it esthetically more pleasing to not update so frequently. I used to just use the draw_delta to achieve the desired update rate relative to the number of items being processed.

@mpetri
Copy link
Author

mpetri commented Jul 31, 2022

Another reason for this would be that it provides smoother estimates around ETC when iterating over elems where processing times fluctuate a bit. Currently I do once per sec updates and ETA jumps from 11hours to 4 hours all the time. With a 5 or 10 sec update rate this would be smoothed out to provide a more accurate estimate.

@djc
Copy link
Collaborator

djc commented Jul 31, 2022

Are the estimates worse than before if you change the refresh rate in the old version?

@Walther
Copy link

Walther commented Aug 16, 2022

Wasn't entirely sure whether to open a new issue, decided to comment here on an existing issue.

After upgrading to 0.17, in my use case the ETA became effectively useless. Two issues:

  1. The numbers jump around way too fast to be readable
  2. As mpetri mentioned above, their value range jumps around way too much to give useful information. On a task that takes approx 3 minutes, the eta_precise keeps jumping around values between less than one minute and over 10 minutes, never converging to a useful mostly-decreasing 3min value.
indicatif-0.17-smaller.mov

Here's a screen recording of 0.16 with a bar.set_draw_delta():

indicatif-0.16-smaller.mov

Compared to 0.16,

  1. The numbers don't twitch around as much, making it much more readable
  2. The ETA stays mostly convergent and mostly monotonically decreasing

Even if the accuracy could be better here (that's one long 30 seconds, heh), 0.16 had better UX on those two aspects.

(Pre-empting the possible suggestion of using eta instead of eta_precise: sadly due to the range issue (2), it too became pretty unreadable (1).)


EDIT:

The 0.17 video above was recorded with no limiting.
I also made a test run with bar.set_draw_target(ProgressDrawTarget::stdout_with_hz(1)); and while it solves some of the readability issues (1), it leaves the jumpy values problem (2) - at every tick, the value can jump pretty wildly compared to the 0.16 behavior. Something is different regarding the convergence between the versions 🤔

indicatif-0.17-1hz-smaller.mov

@djc
Copy link
Collaborator

djc commented Aug 17, 2022

@Walther thanks for the report. I think this is a separate issue, as previously discussed in #394. Please reopen that issue or open a new one. If you can do a bisection of the regression you're seeing, that would be great.

@mpetri
Copy link
Author

mpetri commented Aug 18, 2022

Given the amazing videos @Walther posted I tried to capture my problem as well in this video:

2022-08-18.09-00-51.mp4

which was created with:

  let pb = m.add(ProgressBar::with_draw_target(
      Some(num_nodes as u64),
      indicatif::ProgressDrawTarget::stderr_with_hz(1),
  ));
  pb.set_style(
      ProgressStyle::with_template(
          "{msg} {spinner:.green} [{elapsed_precise}] [{bar:40.cyan/blue}] ({pos}/{len}, ETA {eta}, SPEED {per_sec})",
      )
      .unwrap(),
  );
  pb.set_message(format!("[CREATE CLUSTER {}]", seed));

  • this does not seem to follow the 1 hz update rate
  • the estimate jump around widely (same issue as @Walther )
  • it is just not very pleasing to look at

@djc
Copy link
Collaborator

djc commented Aug 18, 2022

If you had a setup on 0.16 that you did find pleasing to look at, do you have a video of that, too?

@hut8
Copy link

hut8 commented Oct 16, 2022

What do you think about this patch? It shouldn't be backwards incompatible due to trait bounds. I am a beginner at rust but changing this to f64 (or really, Into<f64>) seems like an okay solution. Then I can change the rate to 0.1 to update only once every 10 seconds, for example.

I think this is quite necessary due to the jumpy nature of the values otherwise. Within the span of one second, my progress bar estimates that the ETA is 20 minutes all the way to 1 hour and is impossible to read.

main...hut8:indicatif:float-rate-limit

@hut8 hut8 linked a pull request Oct 16, 2022 that will close this issue
@djc
Copy link
Collaborator

djc commented Oct 16, 2022

Again, if you want to run a lower refresh rate because the estimates jump around too much, you should investigate the estimation code and fix it there. See discussion in #394.

@remi-dupre
Copy link

remi-dupre commented Feb 9, 2024

Hi!

I just came across this issue and it seems that it is idle because no good reason was given why a lower refresh rate would be required.

A while ago at my previous job I built a tool that would run for a very log time with the full dataset as input (at least 24h). With a refresh rate of 1Hz a full day would then output almost 100k lines of logs, which was very inconvenient to work with (truncated logs or slow searches).

At the time it even motivated to build my own lighter progress library (which allowed me to update ~ every 1min) 😕 https://github.com/remi-dupre/prog-rs

@djc
Copy link
Collaborator

djc commented Feb 10, 2024

Well, if someone is sufficiently motivated I might take a PR for this.

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 a pull request may close this issue.

5 participants