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

Default Fetcher.ProgressListener to stderr. #4499

Merged
merged 2 commits into from Apr 21, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 13 additions & 10 deletions src/python/pants/net/http/fetcher.py
Expand Up @@ -145,19 +145,22 @@ def checksum(self):
return self._checksum

class ProgressListener(Listener):
"""A Listener that logs progress to stdout."""
"""A Listener that logs progress to a stream."""

def __init__(self, width=None, chunk_size_bytes=None):
def __init__(self, width=None, chunk_size_bytes=None, stream=sys.stderr):
Copy link
Member

Choose a reason for hiding this comment

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

it'd be great to stream=None and then self._stream = stream or sys.stderr inline in this method.

with the way python default args work, as-is this will latch onto a reference to the value of sys.stderr at the time that this code was imported - which may be the wrong sys.stderr after e.g. a daemon/daemon-runner fork(). deref'ing the value of sys.stderr at runtime vs import time - e.g. in the method body vs as a default arg - will prevent this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point - fixed.

"""Creates a ProgressListener that logs progress for known size items with a progress bar of
the given width in characters and otherwise logs a progress indicator every chunk_size.

:param int width: the width of the progress bar for known size downloads, 50 by default.
:param chunk_size_bytes: The size of data chunks to note progress for, 10 KB by default.
:param stream: A stream to write progress information to; `sys.stderr` by default.
:type stream: :class:`io.RawIOBase`
"""
self._width = width or 50
if not isinstance(self._width, six.integer_types):
raise ValueError('The width must be an integer, given {}'.format(self._width))
self._chunk_size_bytes = chunk_size_bytes or 10 * 1024
self._stream = stream
self._start = time.time()

def status(self, code, content_length=None):
Expand All @@ -178,20 +181,20 @@ def recv_chunk(self, data):
if chunk_count > self.chunks:
self.chunks = chunk_count
if self.size:
sys.stdout.write('\r')
sys.stdout.write('{:3}% '.format(int(self.read * 1.0 / self.size * 100)))
sys.stdout.write('.' * self.chunks)
self._stream.write('\r')
self._stream.write('{:3}% '.format(int(self.read * 1.0 / self.size * 100)))
self._stream.write('.' * self.chunks)
if self.size:
size_width = len(str(self.download_size))
downloaded = int(self.read / 1024)
sys.stdout.write('{} {} KB'.format(' ' * (self._width - self.chunks),
str(downloaded).rjust(size_width)))
sys.stdout.flush()
self._stream.write('{} {} KB'.format(' ' * (self._width - self.chunks),
str(downloaded).rjust(size_width)))
self._stream.flush()

def finished(self):
if self.chunks > 0:
sys.stdout.write(' {:.3f}s\n'.format(time.time() - self._start))
sys.stdout.flush()
self._stream.write(' {:.3f}s\n'.format(time.time() - self._start))
self._stream.flush()

def __init__(self, root_dir, requests_api=None):
"""Creates a Fetcher that uses the given requests api object.
Expand Down
31 changes: 28 additions & 3 deletions tests/python/pants_test/net/http/test_fetcher.py
Expand Up @@ -94,11 +94,13 @@ def test_file_no_perms(self):
self.fetcher.fetch(no_perms, self.listener)

@contextmanager
def expect_get(self, url, chunk_size_bytes, timeout_secs, listener=True):
chunks = ['0123456789', 'a']
def expect_get(self, url, chunk_size_bytes, timeout_secs, chunks=None, listener=True):
chunks = chunks or ['0123456789', 'a']
size = sum(len(c) for c in chunks)

self.requests.get.return_value = self.response
self.response.status_code = 200
self.response.headers = {'content-length': '11'}
self.response.headers = {'content-length': str(size)}
self.response.iter_content.return_value = chunks

yield chunks, [self.ok_call(chunks)] if listener else []
Expand Down Expand Up @@ -284,6 +286,29 @@ def test_download_path(self):
with open(path) as fp:
self.assertEqual(downloaded, fp.read())

@mock.patch('time.time')
def test_progress_listener(self, timer):
timer.side_effect = [0, 1.137]

stream = StringIO()
progress_listener = Fetcher.ProgressListener(width=5, chunk_size_bytes=1, stream=stream)

with self.expect_get('http://baz',
chunk_size_bytes=1,
timeout_secs=37,
chunks=[[1]] * 1024) as (chunks, expected_listener_calls):

self.fetcher.fetch('http://baz',
progress_listener.wrap(self.listener),
chunk_size_bytes=1,
timeout_secs=37)

self.assert_listener_calls(expected_listener_calls, chunks)

# We just test the last progress line which should indicate a 100% complete download.
# We control progress bar width (5 dots), size (1KB) and total time downloading (fake 1.137s).
self.assertEqual('100% ..... 1 KB 1.137s\n', stream.getvalue().split('\r')[-1])


class FetcherRedirectTest(unittest.TestCase):
# NB(Eric Ayers): Using class variables like this seems horrible, but I can't figure out a better
Expand Down