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

Change progress bars to tqdm library #5407

Merged
merged 33 commits into from Jul 1, 2019

Conversation

Projects
None yet
5 participants
@czoido
Copy link
Contributor

commented Jun 27, 2019

Changelog: Feature: Change progress bar output to tqdm to make it look better
Docs: Omit

Closes #3536

Hi!, I have been making some changes to use the same the same progress bars everywhere.
In this branch tqdm library is used for progress bars and
when the bar finishes a [done] message will appear replacing the bar.
Here is the comparison between them:

current_progress
new_progress

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

czoido added some commits Apr 4, 2019

@czoido czoido requested a review from jgsogo Jun 28, 2019

@jgsogo

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

I reallly like it and I think these changes are needed, I'm just wondering if we can take into account a couple of details, feel free to comment and/or discard any of them before trying to make any modification:

  • Console width. I think it should be taken from the actual console width, otherwise the progress bars start to write several lines and the output is a mess.
  • Example of output:
    Downloading conanmanifest.txt
    conanmanifest.txt [done]                                                            
    Downloading conanfile.py
    conanfile.py [done]                                                                 
    Downloading conan_sources.tgz
    conan_sources.tgz [done]                                                            
    Decompressing conan_sources.tgz [done]
    
    Last [done] is in the same line as the description. I prefer this option (more compact) for every line or, if we use a new line I prefer to show the statistics ([size, time to do action])
  • Piping to a file:
    boost/1.70.0@conan/stable: Package installed 6b889d4de82254d8e4d5006bdf79e52e2af2c88c
    boost/1.70.0@conan/stable: Downloading boost/1.70.0@conan/stable:c43a78d3e0e0c6199b377d00bd5385c6d7716729
    boost/1.70.0@conan/stable: Retrieving package c43a78d3e0e0c6199b377d00bd5385c6d7716729 from remote 'conan-center' 
    Downloading conanmanifest.txt
    
    Downloading conaninfo.txt
    
    Downloading conan_package.tgz
    
    ....
    
    
    Can we get rid of those blank lines? I think that a compact output is desired.
  • I'm also trying to check the output of the run_to_file, but I cannot make it work. 🤷‍♂
@czoido

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

Yes, I'm going to give a try to the output you suggest as I think It will be cleaner.
I'm also having a look at the output when not in terminal mode to see if we can get rid of the blank lines.
Thanks a lot for your feedback!

czoido added some commits Jun 28, 2019

@czoido

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

Cleaner output:
modified_bars

@uilianries

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

@czoido Have you tried using CI services? (Gitlab, Travis, Appveyor, Azure...) ?

@czoido

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

Hi @uilianries
I think that It should not be a problem because It always checks that output.is_terminal before
putting the progress bar so the change shouldn't affect CI or tests. Conan Jenkins CI has no
problems with it for the tests.
This is how It should look for CI:

ci_bars

@uilianries

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

I see.

Anyway, I made some validation using your changes with CI services. It's working fine:

https://ci.appveyor.com/project/uilianries/test-conan-new-bar/builds/25614601
https://gitlab.com/uilianries/test-conan-new-bar/-/jobs/242056013
https://travis-ci.com/uilianries/test-conan-new-bar/builds/117301465

There is no progress bar, but it's the expected behavior, as you said.

@czoido

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

That's great! Thanks a lot!

@uilianries
Copy link
Member

left a comment

LGTM

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2019

This is looking good.
Just one quick question, because it was a problem in CI. In CI, it is not only necessary that it doesn't appear a progress bar, but a dot "." should be printed every X (15-30) seconds, because otherwise some CIs automation consider the job stalled. I think this was already a feature of some of our custom progress bars, please @uilianries and @czoido, check with the changes.

I think I would go with this approach to get the functionality with minimal changes, but in a future PR, will try to factor the "is_terminal" checks, and have some abstract "progress-bar" that abstract away the details for different terminals, etc.

@uilianries

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

@czoido you can use https://speed.hetzner.de/ to download bigger files which requires more time and printing the progress dots.

@czoido

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

Hi @memsharded , The CI output is the same than It was before but in one case I have just changed
when uploading files. Now a dot is written for aproximately each uploaded MB. I think It shouldn´t be
a problem but I will do further tests with @uilianries suggestions.
I completely agree to go ahead with minimal changes and factor everything in the near future.
I think we should should homogenize all file handling operations.
The output now should be like this for the CI:
dots_output_ci

@memsharded
Copy link
Contributor

left a comment

I think this is relatively low risk, and looks like a step in the right direction.
As commented before, this deserves a refactor to improve the usage, but I am fine with introducing first the functionality in this slightly "dirtier" ways, it makes clear what changed and what can be affected.

I'd say that it might be even merged for 1.17 release if possible, wdyt @lasote?

@lasote lasote added this to the 1.17 milestone Jul 1, 2019

@lasote

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

Agree, let's do it!

@lasote lasote merged commit 441cb40 into conan-io:develop Jul 1, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.