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

Unzip progress bars #3545

Merged
merged 11 commits into from Oct 8, 2018

Conversation

Projects
None yet
5 participants
@jgsogo
Copy link
Member

commented Sep 13, 2018

  • close #3543
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • Docs not needed
  • Add a test around wrapper FileObjectProgress

I've tried to introduce it in the less invasive way possible.

Example output doing a conan install boost/1.68.0@conan/stable:

Changelog: Feature: Progress bars for files unzipping.

image

@ghost ghost assigned jgsogo Sep 13, 2018

@ghost ghost added the stage: review label Sep 13, 2018

@jgsogo jgsogo requested review from memsharded and lasote Sep 13, 2018

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

This is the less invasive way to introduce tqdm for decompressing jobs, but if we are going to adopt tqdm everywhere maybe there are better ways to go.

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

BTW, we have to investigate the 17.6MB vs 16.8M file sizes 😕

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

I'm thinking about forwarding all unknown attributes/methods to the underlying self._fileobj, or inheriting from io.FileIO and override just the save method... do you think it is a clean approach?

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

Mb difference might be if using SI or not.

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

Given the output here, I'm sure we have to disable progress bars on CI systems, too much noise (use tqdm(..., disable=True))

from tqdm import tqdm
from contextlib import contextmanager

TIMEOUT_BEAT_SECONDS = 30

This comment has been minimized.

Copy link
@jgsogo

jgsogo Sep 13, 2018

Author Member

Do we want to make this value configurable? It is used to print a heartbeat character instead of the progress bar. Which value to use?

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 13, 2018

Contributor

If that is for the "dot" being printed every few seconds, both for the user and CI not aborting, I would say hardcode it by now, to something that makes sense from a UI perspective (every 10 seconds?)

This comment has been minimized.

Copy link
@jgsogo

jgsogo Sep 13, 2018

Author Member

This is the "dot" being printed only if not output.is_terminal, so tipically it will be shown in CI and tests.

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

Using the ConanOutput class, it seems that tqdm can't get information about the console size (in fact there is no method in our class to retrieve it), and it is using some kind of default size:

image

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

There is a bug (py2) in this version of tqdm, output shows the following:;

Exception KeyError: KeyError(<weakref at 0x7f5cd8944fc8; to 'tqdm' at 0x7f5cd8a1c650>,) in <bound method tqdm.__del__ of Decompressing conan_export.tgz: 18.0B [00:00, 5.84kB/s]> ignored

Issue reported here: tqdm/tqdm#549 (PR ongoing: tqdm/tqdm#607)

Solution: may downgrade to a working version; or I think that catching the exception, closing the pb and re-raising it could work (in the open_binary function):

@chengs

This comment has been minimized.

Copy link

commented Sep 14, 2018

@jgsogo, I am a developer of tqdm, let's wait for my PR to be merged. Currently, if you use py2 with tqdm, tqdm v4.20 should be fine (or v4.19)

Catching this exception (in tqdm>=4.21) might be tricky.

@memsharded
Copy link
Contributor

left a comment

Looks pretty good. Are the py2 issues with the tqdm lib already solved, is that PR already merged and released?

@@ -29,6 +29,7 @@
from conans.util.env_reader import get_env
from conans.search.search import filter_packages
from conans.client.cmd.uploader import UPLOAD_POLICY_SKIP
from conans.util import progress_bar as pb

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 20, 2018

Contributor

I think I'd prefer the full progress_bar, explicit name.


def __init__(self, fileobj, output, desc=None):
pb_kwargs = self.tqdm_defaults.copy()
self.ori_output = output

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 20, 2018

Contributor

make ori_output private?

TIMEOUT_BEAT_CHARACTER = '.'


class BinaryFileWrapper(object):

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 20, 2018

Contributor

A class name more descriptive of its purpose, like ProgressBarFileReader

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2018

About the py2 issue, @chengs' PR is pending yet. Maybe we should downgrade the tqdm version in order to test this progress bar in the next release.

@lasote lasote added this to the 1.9 milestone Sep 24, 2018

@jgsogo jgsogo referenced this pull request Sep 24, 2018

Closed

Functions for upload/download with progress bars #3608

3 of 3 tasks complete

@jgsogo jgsogo removed this from the 1.9 milestone Sep 25, 2018

@jgsogo jgsogo added this to the 1.8 milestone Sep 25, 2018

@lasote lasote changed the title POC for progress bars using tqdm Unzip progress bars Sep 25, 2018

@jgsogo jgsogo removed their assignment Sep 27, 2018

self._pb = tqdm(total=self.tell(), desc=desc, file=output, **pb_kwargs)
self.seek(0)

def seek(self, *args, **kwargs):

This comment has been minimized.

Copy link
@SSE4

SSE4 Oct 3, 2018

Contributor

should it implement other fileojb methods, e.g. fileno?

This comment has been minimized.

Copy link
@jgsogo

jgsogo Oct 3, 2018

Author Member

Hi! This is just a proof of concept (to see if someone reports a weird behavior in CI or interactive). The implementation will be far more complete (following this idea: #3608), but probably it will remain only for binary files (files opened in binary mode) as this is the type of content be have to deal with in Conan.

@lasote lasote merged commit c9e833f into conan-io:develop Oct 8, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Oct 8, 2018

@jgsogo jgsogo deleted the jgsogo:issue/3543 branch Oct 8, 2018

@alacasta alacasta referenced this pull request Dec 3, 2018

Closed

Conan >=1.8 fails when called from SCons 2.4.1 #4046

3 of 3 tasks complete

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Unzip progress bars (conan-io#3545)
* POC for progress bars using tqdm

* remove unused imports

* add seek function

* support only binary read, add a contextmanager to better control flow

* remove unused argument

* new line in non-terminal progress bar

* global var for the timeout char

* remove invalid file

* flush has no params

* more meaninful names to classes and variables

* downgrade tqdm to 4.20 to avoid error message
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.