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 maximum_refresh_time to progress_bar() #147

Merged
merged 2 commits into from
Jul 5, 2024
Merged

Conversation

hagenw
Copy link
Member

@hagenw hagenw commented Jul 1, 2024

This adds maximum_refresh_time which allows to ensure that a progress bar is refreshed at least every maximum_refresh_time seconds.

This is of interest when the time between the single steps of the progress bar iteration take very long, and a user might start to wonder if the process is stalled or still running.

The refreshing is achieved by using another thread as proposed in tqdm/tqdm#861 (comment). To not change and possible break any existing code, the default value of maximum_refresh_time is None which will not force refreshing the progress bar. I would propose we set this value then to an actual value in applications where it is relevant, e.g. audb.

For a local example run:

import time

import audeer


for _ in audeer.progress_bar(range(2)):  # old behavior
    time.sleep(4)

which results in a display of 00:00 for a long time:

image

and compare with

for _ in audeer.progress_bar(range(2), maximum_refresh_time=1):
    time.sleep(2)

which updates the already past time every second:

image

Updated docstring:

image

Copy link

codecov bot commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.0%. Comparing base (ec72190) to head (ff19061).

Additional details and impacted files
Files Coverage Δ
audeer/core/tqdm.py 100.0% <100.0%> (ø)

@hagenw hagenw marked this pull request as draft July 1, 2024 08:20
@hagenw hagenw force-pushed the enforce-tqdm-refresh branch from 9f08e48 to ff19061 Compare July 1, 2024 09:12
@hagenw hagenw marked this pull request as ready for review July 1, 2024 09:17
@hagenw hagenw requested a review from ChristianGeng July 1, 2024 09:17
Copy link
Member

@ChristianGeng ChristianGeng left a comment

Choose a reason for hiding this comment

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

Review

The MR allows to add an additional param that will refresh the progress bar even when an iteration takes confusingly wrong. Like this the user can be more confident, seeing that the pbar is still alive.

Threading

I will be upfront with slappping the label "noob" onto my knowledge level when it comes to threading .-). Saying that I think I understand that the refresh thread specified as a daemon thread does not have to be joined, as it will die anyway when the main thread exits, and does not continue existing as a zombie.
At least this comment to me suggtests that this is ok.

Implementation

I wonder how this relates to the parameters args of tqdm.tqdm , see here?
Would not the parameters miniters, mininterval, maxinterval allow to create identical or at least similar behavior?

@hagenw
Copy link
Member Author

hagenw commented Jul 1, 2024

I wonder how this relates to the parameters args of tqdm.tqdm , see here?
Would not the parameters miniters, mininterval, maxinterval allow to create identical or at least similar behavior?

Yes, you would expect it, but I never got maxinterval to work. There are also issues about that, see tqdm/tqdm#861 (comment) and tqdm/tqdm#717. The implementation I used here was presented as a possible solution to the first issue at tqdm/tqdm#861 (comment)

@hagenw
Copy link
Member Author

hagenw commented Jul 1, 2024

The better solution would be to fix tqdm to respect maxinterval, but I'm too lazy to figure out how to do it ;)

@hagenw
Copy link
Member Author

hagenw commented Jul 1, 2024

Saying that I think I understand that the refresh thread specified as a daemon thread does not have to be joined, as it will die anyway when the main thread exits, and does not continue existing as a zombie.

Honestly, I also have no clue. I will try to investigate this first, before merging here.

@ChristianGeng
Copy link
Member

The better solution would be to fix tqdm to respect maxinterval, but I'm too lazy to figure out how to do it ;)

With the problems that exist using m̀axinterval` I would say that the implementation is ok.

Honestly, I also have no clue. I will try to investigate this first, before merging here.

In anticipation I will approve the MR in order to apply my new knowledge on how to re-revie and then approve ,-)

Copy link
Member

@ChristianGeng ChristianGeng left a comment

Choose a reason for hiding this comment

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

Redoing the review, this MR is approved as

  • the discussion about the tqdm has been settled with the result that this is a workaround needed to overcome the current tqdm status
  • the daemon parameter of the refresh callback is being checked and can be fixed later in the unlikely case this is needed.

@hagenw
Copy link
Member Author

hagenw commented Jul 5, 2024

I tested if the thread goes away with the following code

import threading
import time

import audeer


print(f"Number of threads before progress bar: {threading.active_count()}")
print("Active threads:")
for thread in threading.enumerate():
    print(thread)
print()
for _ in audeer.progress_bar(range(2), maximum_refresh_time=0.01):
    time.sleep(0.05)
    print(f"Number of threads during progress bar: {threading.active_count()}")
    print("Active threads:")
    for thread in threading.enumerate():
        print(thread)
print()
print(f"Number of threads after progress bar: {threading.active_count()}")
print("Active threads:")
for thread in threading.enumerate():
    print(thread)

We get one additional thread, when starting with a fresh Python console, after the progress bar, but this is a generic monitoring thread. The thread that refreshes the progress bar is only active when the progress bar gets updated:

Number of threads before progress bar: 1
Active threads:
<_MainThread(MainThread, started 135948201152512)>

Number of threads during progress bar: 3
<_MainThread(MainThread, started 135948201152512)>
<TMonitor(Thread-1, started daemon 135948176623168)>
<Thread(Thread-2 (refresh), started daemon 135948168230464)>
Number of threads during progress bar: 3
<_MainThread(MainThread, started 135948201152512)>
<TMonitor(Thread-1, started daemon 135948176623168)>
<Thread(Thread-2 (refresh), started daemon 135948168230464)>

Number of threads after progress bar: 2
Active threads:
<_MainThread(MainThread, started 135948201152512)>
<TMonitor(Thread-1, started daemon 135948176623168)>

@hagenw hagenw merged commit d936e64 into main Jul 5, 2024
19 checks passed
@hagenw hagenw deleted the enforce-tqdm-refresh branch July 5, 2024 08:27
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.

2 participants