-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Multiple bars display fix #1054
base: master
Are you sure you want to change the base?
Conversation
if self.display(msg='', pos=pos) and not pos: | ||
if self.display(msg='', pos=pos): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not 100% sure about this change. I don't understand what is and not pos
here for, but it affects the fourth example from the PR description (with leave=False
): it leaves an unexpected newline symbol
pos = abs(self.pos) | ||
self._decr_instances(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have to combine the locking inside of _decr_instances
and the locking located some lines below to avoid a race condition, which may result in 2 (or more) bars terminating lines, while only the very last one must
This does fix the issue that I raised (when I use Also, the nested loop example is still strange IMO. I would expect based on the documentation that all 10 bars should remain after execution (since |
@haji-ali, thank you for the constructive feedback!
I expect
I don't think, this is a feature because the following sample produces a similar output for the current version of for _ in trange(10, position=0):
for _ in trange(10, position=1):
pass I think this following part from the docs is supposed to tell that the sample from the PR's description should be interpreted just like the above sample (however, that's not obvious at all, I'd like to get a confirmation or a denial from @casperdcl): Lines 918 to 920 in 8f9e03d
But even if it's not a bug, it should be fixed in a separate PR, because this is an issue with the _get_free_pos method, used to figure out the position of a bar automatically:Line 1079 in 8f9e03d
|
I tried this PR with my nested multi-thread long function that I've been having issues with, and it seems to resolve most of my problems.
Now I only have to figure out a way to count up the inner loop time spent before it is completed. |
fa4de6b
to
687c6b7
Compare
c06148c
to
661d467
Compare
Hello, @casperdcl. I'm sorry for bothering you, but I wonder if there is any news on this? |
Ok finally reviewed this (!) Referring to the screenshots:
ok
this is identical to setting
Same as above - bars are "left" (i.e. printed like normal lines) as they are completed.
The whitespace causing a newline in the current version is odd - looks like a bug.
I'm not too concerned by this as both versions look bad to me. If someone manually specifies position in a naïve way (both cases) then the output will look bad. |
@casperdcl, thanks! :)
Oops, looks like I misunderstood the
???
Not sure if I understood you correctly. Which both of the three (Before, After, After edit) are you talking about? |
from time import sleep
print(sleep(1), "task 3 completed")
print(sleep(1), "task 2 completed")
print(sleep(1), "task 1 completed") will print statements in order of completion (3, 2, 1). The same is true of any bar which has from time import sleep
from tqdm import tqdm
for i in tqdm([1], desc="task 1"):
for j in tqdm([1], desc="task 2"):
for k in tqdm([1], desc="task 3"):
sleep(1) The final order is (3, 2, 1). The fact that while the bars are "running" the transient order is (1, 2, 3) is besides the point. If you would like to maintain this transient order even after the bars have closed (i.e. print final order (1, 2, 3)), this is identical to expecting The easiest way may be to set bars = [tqdm(..., leave=False), ...]
# do some work, update/close bars, etc...
# print them all
for bar in bars:
print(bar) |
This is questionable. In my opinion, the standard behaviour for bars is to not be overwritten by other bars if |
I personally find the behavior with the patch more intuitive. The nested loop behavior is especially confusing. The bars being overwritten and unfinished bars being left over makes it difficult to tell the progress if I am not following how the new bars are being created. |
Looks like the discussion is stuck. Is there some well-established mechanism to find out the opinion of users on such questions? How do we figure out which of these do people actually find more intuitive? |
@casperdcl?..
|
I have checked this PR. If we do not set position argument and use a semaphore, the code works incorrectly. The code example: from concurrent.futures import ThreadPoolExecutor, wait
from threading import Semaphore
import time
import random
from tqdm import tqdm
def worker(pos, sem):
t = random.random() * 0.05
with sem:
# for _ in tqdm(range(100), desc=f'pos {pos}', position=pos): # it works correctly
for _ in tqdm(range(100), desc=f'pos {pos}'): # it works incorrectly
time.sleep(t)
def main():
with ThreadPoolExecutor() as executor:
sem = Semaphore(3)
futures = []
for pos in range(10):
future = executor.submit(worker, pos, sem)
futures.append(future)
wait(futures)
if __name__ == '__main__':
main() The new bars overwrite the old bars if we do not set |
Hello! I'm back here. Sincerely sorry for such a long delay, I start getting closer to removing the Busy mark from my profile.
@espdev, thank you so much for pointing it out. I'm going to fix this (hopefully, soon). In the model which I propose in this PR, this bug seems to be caused by the logic of Method's source: Lines 579 to 584 in 140c948
Example code: from time import sleep
from threading import Thread
from tqdm import tqdm
def run_me(f):
for _ in tqdm(list(range(20)), file=f):
sleep(0.1)
if __name__ == "__main__":
f = open("/tmp/f1", "w")
t1 = Thread(target=run_me, args=(f,))
t2 = Thread(target=run_me, args=(None,))
t1.start()
t2.start()
t1.join()
t2.join() Result of the code execution: https://asciinema.org/a/431831 |
Fixes bugs when displaying multiple bars when running nested loops or providing `position` argument. Downgrade: after bars are closed, the cursor is not moved below them, resulting in future prints problems.
661d467
to
2e36fe6
Compare
Whenever a group of bars (e.g. from nested loops, parallel execution, etc) finishes (which means all the bars are closed), cursor moves below the printed bars so that it's possible to print something after them. Note: the behavior has also changed for the case when there are bars with positions specified, which run non-simultaneously. Every time a bar is created for a file where there are no bars running, the positions are numbered from the current cursor position. Could be a subject to fix later
2e36fe6
to
6d7f57b
Compare
This allows to produce expected output when using tqdm bars with `position` parameter not concurrently
6d7f57b
to
9395182
Compare
@casperdcl, I have one more question regarding the expected bars' behavior. By the reaction you have left I can tell that you agree that the behavior found by espdev is incorrect. I assume that in that case it was expected that new bars appear under the completed ones. But I wonder about one more complicated case. Imagine there were three bars created: first with |
So far, I use Numba to help me achieve multi-progress bars display with multi-threads. But this cannot be implemented via the python multi-process library since communication between different processes is not straightforward. Therefore, I wonder if it is possible for tqdm to maintain a unique internal pool for multi-progress bars in the master process while other processes can send a signal to update those bars. Then it would be easier for users to manage the multiple progress bars not only for multi-thread usage but also for complicated scenarios. Please check my naive implementation.
|
I believe the solutions to the problem exist, but I consider them going out of the frame of this PR: here I only focus on problems with bars positioning caused by logical bugs and don't focus on processes' synchronization. You might want to discuss this in another (a new one?) issue/PR.
To be honest, I wouldn't say the implementation you've provided is easy to use: I see that you need to manually call the
I am not familiar with If you want to work on this problem, if I were you, I'd start by either finding or filing an issue with a detailed explanation of the problem you want to solve and code samples (preferably - using only standard libraries (like |
Codecov Report
@@ Coverage Diff @@
## master #1054 +/- ##
=======================================
Coverage 89.05% 89.05%
=======================================
Files 26 26
Lines 1763 1763
Branches 344 344
=======================================
Hits 1570 1570
Misses 144 144
Partials 49 49 |
Thanks for your reply. |
5116501 worked perfectly for me in PyCharm on Ubuntu using "Emulate terminal in output console". Thank you! |
@rckoepke, glad to hear that, thank you for your feedback! Why did you use this commit, not the top of the branch? Does the behavior somehow worsen in the following commits or you just haven't tested? |
To be honest, I independently found the same two-line solution and it was sufficient for my use case. I went to submit a issue / PR for it and found this existing PR that already had the same solution, plus apparently more comprehensive coverage. It looked a bit stale and I so I wanted to provide additional positive feedback. The other commits look very helpful and certainly better than the original two-line solution that I came up with, but I'm not sure how I would rigorously test them. I do have Windows+Mac+Linux and a variety of IDE's and terminal environments, so I'd be happy to do cross-platform testing if there was some kind of a test matrix that I could follow. |
Oops, no that's not what I meant. Retracted the 👍. What I mean is the output of the code is as expected. The Here's my output of auto-position (unaltered copy-paste of this code): If this is counter-intuitive it probably bears documenting.
yes, the Numba implementation is interesting but I haven't had a look at it yet. |
@kolayne @casperdcl Came across this from #1000. Any update on this? |
I also stumbled across this issue when using multiprocessing. The example in the README still does not work. Any update on when a solution might be merged into a stable release? |
About
Fixes several issues with bars positioning when using multiple bars. Resolves #1000 and probably some other similar issues.
Other solutions
I know there already exists a PR fixing the problem (#1001), but I don't like it for a few reasons, including:
Changed and unchanged behaviour
Here there are samples I was running to compare the modified
tqdm
with the original one. All the samples were run on Python withsys.platform == 'linux'
,sys.version == '3.8.5 (default, Jul 28 2020, 12:59:40) \n[GCC 9.3.0]'
. The "before" screenshots are run ontqdm
installed frommaster
, the "after" screenshots are run ontqdm
installed from the current branch (bars_position_fix
).Trivial
(Just to verify I haven't broken the very basics)
Before
After
Nested loops
Before
After
Multithreaded with `position`
Please, pay attention to the order of the bars (numbers in the beginnings of the lines): in the version before the PR bars change their order. Sometimes (when using enough many bars or running them long enough) a similar sample can also produce empty lines or duplicate bars (if you want, I can try to dig into it and provide such a sample)
Before
After
One-threaded with `position` and `leave=False`
Before
After
One-threaded with `position`
This is the only case (I can think of) where you have to change code to make it produce a perfect output (if not changing it, the produced output is still better, than the output before the PR, as for me). However, I don't think this is a serious issue, because I think, this case is extremely special: there is no need to use bars with
position
if the application is one-threaded. In theory, there is one more situation where this case may come (when you use a multi-threaded application, but for some reason, a subset of the bars you want to use both starts and closes before other bars are created).Before
After
Updated sample:
After edit
Tests
I have very little experience with tests (both in python and generally). My PR has obviously changed
tqdm
outputs in some cases, so I think some tests will fail and I'll need help from maintainers (@casperdcl?) to understand, where I should change something back and where tests should be updated instead