Skip to content

End TextProgressBar with newline (#3414)#5968

Merged
jacobtomlinson merged 1 commit intodask:mainfrom
vepadulano:progress-write-newline
Aug 22, 2022
Merged

End TextProgressBar with newline (#3414)#5968
jacobtomlinson merged 1 commit intodask:mainfrom
vepadulano:progress-write-newline

Conversation

@vepadulano
Copy link
Copy Markdown
Contributor

@vepadulano vepadulano commented Mar 21, 2022 •
edited
Loading

So that the progress bar diagnostic doesn't interfere with other print statements or logging by the user. I stumbled upon the same problem reported in the linked issue, this is my reproducer for it:

import time
import random

from dask import delayed
from dask.distributed import Client, LocalCluster, progress


def inc(x):
    time.sleep(random.random())
    return x + 1

def dec(x):
    time.sleep(random.random())
    return x - 1

def add(x, y):
    time.sleep(random.random())
    return x + y

inc = delayed(inc)
dec = delayed(dec)
add = delayed(add)

if __name__ == "__main__":
    cluster = LocalCluster(n_workers=2, threads_per_worker=1, processes=True)
    client = Client(cluster)

    x = inc(1)
    y = dec(2)
    z = add(x, y)
    print("Starting computation")
    res = z.persist()
    progress(res)
    print(f"Computation ended, result is {res.compute()}")

The output

$: python test_dask_delayed.py 
Starting computation
Computation ended, result is 3###########] | 100% Completed |  1.5s

After this PR

$: python test_dask_delayed.py 
Starting computation
Computation ended, result is 3

@GPUtester
Copy link
Copy Markdown
Collaborator

GPUtester commented Mar 21, 2022

Can one of the admins verify this patch?

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 21, 2022

Unit Test Results

       12 files  ±    0         12 suites  ±0   6h 52m 54s ⏱️ + 41m 24s
  2 667 tests ±    0    2 578 ✔️  -     4    82 💤 +  2    7 ❌ +  2 
15 188 runs  +719  14 334 ✔️ +664  826 💤 +34  28 ❌ +21 

For more details on these failures, see this check.

Results for commit 301149f. ± Comparison against base commit 1da5199.

@vepadulano
Copy link
Copy Markdown
Contributor Author

vepadulano commented Aug 18, 2022

Hi! Any chance to have a quick look at this? It is a small change, but it keeps bothering me every time I use the progress bar

jacobtomlinson
jacobtomlinson reviewed Aug 19, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jacobtomlinson jacobtomlinson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for raising this. It seems like this only happens when the printed output is shorter than the progress bar.

Perhaps instead of switching to \n we should change to \33[2K\r which would clear the line.

@vepadulano
Make TextProgressBar clear the line when finished (dask#3414)
d0d1c50 
So that the progress bar diagnostic doesn't interfere with other print
statements or logging by the user.
@vepadulano vepadulano force-pushed the progress-write-newline branch from 301149f to d0d1c50 Compare August 20, 2022 13:21
@vepadulano
Copy link
Copy Markdown
Contributor Author

vepadulano commented Aug 20, 2022

Perhaps instead of switching to \n we should change to \33[2K\r which would clear the line.

Good point, I agree! I modified the commit accordingly

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 20, 2022

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   6h 33m 21s ⏱️ + 2m 52s
  3 037 tests ±0    2 950 ✔️  - 2    83 💤 ±0  2 ❌ ±0  2 🔥 +2 
22 464 runs  +1  21 490 ✔️  - 2  969 💤 +1  3 ❌ ±0  2 🔥 +2 

For more details on these failures and errors, see this check.

Results for commit d0d1c50. ± Comparison against base commit a80187a.

jacobtomlinson
jacobtomlinson approved these changes Aug 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

This looks great thanks!

@jacobtomlinson jacobtomlinson merged commit fe0f4b7 into dask:main Aug 22, 2022
@jakirkham
Copy link
Copy Markdown
Member

jakirkham commented Aug 22, 2022

ok to test

(admittedly a bit late)

gjoseph92 pushed a commit to gjoseph92/distributed that referenced this pull request Oct 31, 2022
@vepadulano @gjoseph92
Make TextProgressBar clear the line when finished (dask#3414) (dask#5968
f3b795a 
)

So that the progress bar diagnostic doesn't interfere with other print
statements or logging by the user.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacobtomlinson jacobtomlinson jacobtomlinson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TextProgressBar and logging/print

4 participants

@vepadulano @GPUtester @jakirkham @jacobtomlinson