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

tqdm to logging #1172

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

tqdm to logging #1172

wants to merge 11 commits into from

Conversation

de-code
Copy link
Contributor

@de-code de-code commented May 29, 2021

see #313
implementation of #313 (comment)

/cc @casperdcl

@de-code de-code requested a review from casperdcl as a code owner May 29, 2021 08:08
@codecov
Copy link

codecov bot commented May 29, 2021

Codecov Report

Merging #1172 (4bf1107) into master (140c948) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1172      +/-   ##
==========================================
+ Coverage   89.68%   89.86%   +0.18%     
==========================================
  Files          26       26              
  Lines        1735     1767      +32     
  Branches      287      293       +6     
==========================================
+ Hits         1556     1588      +32     
  Misses        133      133              
  Partials       46       46              

fixed comma in type hint

added logging_tqdm

added documentation to class

added documentation to README

python 2

flake8

also test with default logger

isort

fixed readme title underline too short

improved coverage
@brtasavpatel
Copy link

@de-code Progress bar update shows up in the new line. Is this expected?

image

@de-code
Copy link
Contributor Author

de-code commented May 31, 2021

@de-code Progress bar update shows up in the new line. Is this expected?

Yes, that is intentional (except for maybe the last line, not sure why it is logging the same progress twice - could add a check for that). It is passing the progress to the regular logging module. No magic there. On the plus side, in a log file it will also still look sensible.
You might be looking for a different functionality.

@brtasavpatel
Copy link

brtasavpatel commented Jun 1, 2021

@de-code Progress bar update shows up in the new line. Is this expected?

Yes, that is intentional (except for maybe the last line, not sure why it is logging the same progress twice - could add a check for that). It is passing the progress to the regular logging module. No magic there. On the plus side, in a log file it will also still look sensible.
You might be looking for a different functionality.

I am looking for a solution where my progress bar won't look weird in kubernetes logs, at the same time my progress bar is usually iterating over a million files or so that means this will add lot of lines in my logs.

@de-code
Copy link
Contributor Author

de-code commented Jun 1, 2021

I am looking for a solution where my progress bar won't look weird in kubernetes logs, at the same time I am my progress bar is usually for a million files or so that means this will add lot of lines in my logs.

I think this is where the mininterval comes in handy (core feature). With the logging_tqdm in this PR, the default is set to 1 (i.e. 1 second). Which I found personally useful, but you could increase that further. Therefore it doesn't matter how many rows you process but how frequent you want to see a new log message.

What I haven't tested is whether the mininterval is respected when updating the progress description (if that is what you are doing).

@casperdcl
Copy link
Sponsor Member

casperdcl commented Jun 2, 2021

I think @brtasavpatel is referring to duplicating the output: once (normally) to stdout as well as again to a log file

@de-code
Copy link
Contributor Author

de-code commented Jun 6, 2021

I've updated the PR to avoid logging the same message twice (which I think may have happened at the end).

Regarding the logging, I think the confusion came from the sleep in #1172 (comment) being the same as (or larger than) mininterval, 1 second. Which results in a message for about every item. That gives the impression that a message is always logged for every item. But if say mininterval was set to one minute, it would only log once every minute (independent of how many items it processed within that period).

In my example I had the sleep set to 0.3. I have now added a comment to the example and also explicitly include mininterval. Maybe the comment could be expanded further.

import logging
from time import sleep
from tqdm.contrib.logging import logging_tqdm

LOG = logging.getLogger(__name__)

if __name__ == '__main__':
    logging.basicConfig(level=logging.INFO)
    for _ in logging_tqdm(range(10), mininterval=1, logger=LOG):
        sleep(0.3)  # assume processing one item takes less than mininterval

EDIT: the example was wrong (it mixing usage as context manager and iterable)

@de-code
Copy link
Contributor Author

de-code commented Jun 6, 2021

Would it be worth having something like a tqdm.auto that would select logging_tqdm if stderr doesn't seem to be tty?

@de-code
Copy link
Contributor Author

de-code commented Aug 23, 2021

Is this still desired?

@brtasavpatel
Copy link

I would definitely like this to be added in

@de-code
Copy link
Contributor Author

de-code commented Aug 23, 2021

Okay, I merged with master.
Let me know if anything is missing.

@tesfaldet
Copy link

Is this still being worked on? I'd love to have this feature as well for use with logging in Hydra (which uses python's logging stdlib) when I need to check the log files from experiment sweeps.

@de-code
Copy link
Contributor Author

de-code commented Mar 18, 2022

Hi @casperdcl just wondering anything should be done from my side in order to get this merged?

@de-code
Copy link
Contributor Author

de-code commented Jul 9, 2022

Oh dear, I missed the anniversary of this PR. 🎂

Hi @casperdcl it seems some people are interested in getting this merged.
It would be good to hear back either way.

@stdavis
Copy link

stdavis commented Jul 21, 2023

I used this code in my project and it works great! I would love to see this merged!

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.

None yet

5 participants