Skip to content

Fix flush() in clean() and refresh() #331

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix flush() in clean() and refresh() #331

wants to merge 2 commits into from

Conversation

lrq3000
Copy link
Member

@lrq3000 lrq3000 commented Jan 1, 2017 •
edited
Loading

Continuation of #327 by @deeenes.

We now use status_printer() in clean() and refresh(), so not only this automatically does flush(), but it also saves the last print length, which will provide a more consistent display after executing clean() or refresh() and then displaying the bar again (I guess doing clean() and update() in this order might produce weird displays sometimes, this should be fixed by this PR).

Just one test for write() had to be adapted, I checked myself with old example scripts using write() in various sceniarios, everything looks OK.

TODO:

  • Nothing.

deeenes and others added 2 commits January 1, 2017 20:59
@deeenes @lrq3000
Add flush() in refresh()
071ac0e 
without `self.fp.flush()`, refresh always fails in Python 3.5 (but not in 2.7), even with the most mimal test code.  added `flush` at the end of `refresh`.
@lrq3000
Use status_printer() in clean() and refresh() for auto flush()
5fe0621 
Signed-off-by: Stephen L. <lrq3000@gmail.com>
@lrq3000 lrq3000 added p2-bug-warning ⚠ Visual output bad to-merge ↰ Imminent labels Jan 1, 2017
@lrq3000 lrq3000 mentioned this pull request Jan 1, 2017
Closed
@lrq3000 lrq3000 added this to the v5.0.0 milestone Jan 1, 2017
@lrq3000 lrq3000 mentioned this pull request Jan 5, 2017
Closed
casperdcl
casperdcl reviewed Jan 5, 2017
View reviewed changes
Copy link
Member

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

Fine by me provided the comment is addressed.

tqdm/_tqdm.py
@@ -1049,8 +1049,7 @@ def clear(self, nomove=False):
if not nomove:
self.moveto(self.pos)
# clear up the bar (can't rely on sp(''))
self.fp.write('\r')
self.fp.write(' ' * (self.ncols if self.ncols else 10))
self.sp('')
Copy link
Member

Choose a reason for hiding this comment

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

is L1052 sufficient despite the warning on L1051?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I totally missed the comment! All the unit tests pass fine and my own test scripts too, so I don't know nor remember at all why sp couldn't be relied on. I'm going to use git blame and git pickaxe to trace back this comment and see whether I can get more info.

@lrq3000 lrq3000 added to-fix ⌛ In progress and removed to-merge ↰ Imminent labels Jan 12, 2017
@lrq3000
Copy link
Member Author

lrq3000 commented Jan 12, 2017

Just a quick update, please don't merge this one yet. I've investigated a bit and tried to use sp() everywhere instead of direct writing, but 2 test are now failing, the output being different, and it's not really obvious why. I guess the unreliability of sp('') is still true but I don't know why as it should work, I need to hunt down this bug some more!

@casperdcl
Copy link
Member

casperdcl commented Jan 13, 2017 •
edited
Loading

Thanks. I recall writing this code a long time ago and there was a good reason. I think. Which was why I asked. It's better if you take a look at it for a second opinion.

@casperdcl casperdcl mentioned this pull request Jan 20, 2017
Open
@lrq3000 lrq3000 mentioned this pull request Feb 16, 2017
Closed
@casperdcl casperdcl assigned lrq3000 and unassigned casperdcl Mar 3, 2017
@casperdcl casperdcl mentioned this pull request May 26, 2017
Closed
@staffanm staffanm mentioned this pull request Jun 14, 2017
Closed
@casperdcl casperdcl force-pushed the master branch 6 times, most recently from 6ec00f1 to 4b6476a Compare July 22, 2017 14:15
@DrJimFan
Copy link

Is this PR gonna be merged soon? I think it's the cause for #420 as well (in the latest 4.15 release).

@lrq3000 lrq3000 mentioned this pull request Sep 22, 2017
Merged
@lrq3000
Copy link
Member Author

lrq3000 commented Sep 22, 2017 •
edited
Loading

As an alternative to this PR, we could just use self.fp.flush(), this would basically have the same goal (without the weird side effects of self.sp('')).

@MPagel
Copy link
Contributor

MPagel commented Sep 22, 2017

I tried self.fp.flush() first, but that didn't seem to work in a multithreading environment (eventlet) where I was catching all output to stdout/err as per documentation. The following seems to work:

def flush(self):
        fp_flush = getattr(self.file, 'flush', lambda: None)
        fp_flush()

See also #439 #329 ab1bd78

casperdcl added a commit that referenced this pull request Sep 26, 2017
@casperdcl
bump version, Merge branch 'parallel-print-fix2'
e32428c 
fixes #285 -> #291 -> #329
fixes #422
fixes #439

fixes #323
fixes #324
fixes #334
fixes #407
fixes #418

related to:
- #97
- #143
- #331
- #361
- #384
- #385
- #417
casperdcl added a commit that referenced this pull request Oct 5, 2017
@casperdcl
final sp() cleanup
acdcf0e 
supercedes #331
@casperdcl
Copy link
Member

casperdcl commented Oct 5, 2017 •
edited
Loading

could you try the devel branch to verify it fixes this issue? It's slightly different there.

@casperdcl casperdcl mentioned this pull request Oct 5, 2017
Merged
5 tasks
casperdcl added a commit that referenced this pull request Oct 5, 2017
@casperdcl
final sp() cleanup
533b891 
supercedes #331
@casperdcl casperdcl deleted the flush-fix branch May 3, 2021 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperdcl casperdcl casperdcl left review comments

Assignees

@lrq3000 lrq3000

Labels
p2-bug-warning ⚠ Visual output bad to-fix ⌛ In progress
Projects
None yet
Milestone
Non-breaking
Development

Successfully merging this pull request may close these issues.
5 participants
@lrq3000 @casperdcl @DrJimFan @MPagel @deeenes