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

Fix progressbar of fetcher #1015

Merged
merged 4 commits into from Mar 30, 2016

Conversation

Projects
None yet
2 participants
@ghoshbishakh
Member

ghoshbishakh commented Mar 23, 2016

Fallback to metadata download size when content-length is not defined in http headers

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 23, 2016

stops from crashing and falls back to hardcoded download size #1014

response_size = opener.headers['content-length']
try:
response_size = opener.headers['content-length']
except:

This comment has been minimized.

@arokem

arokem Mar 23, 2016

Member

Please specify the exception type. For example:

except KeyError:
@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 23, 2016

@arokem I hope this is better 😅

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 23, 2016

okey!

else:
# python3.x
response_size = opener.getheader("Content-Length")

try:

This comment has been minimized.

@arokem

arokem Mar 23, 2016

Member

Here as well.

if(response_size is None):
copyfileobj(opener, data)
else:
copyfileobj_withprogress(opener, data, response_size)

This comment has been minimized.

@arokem

arokem Mar 23, 2016

Member

Is there some way to rewrite this to pass the data size from the metadata into here, so that you still get a progress bar?

This comment has been minimized.

@ghoshbishakh

ghoshbishakh Mar 23, 2016

Member

yes but that may lead to some weird appearence as the size is not accurate.
I will try and test that maybe tomorrow.

And most of the datasets with meta data tag I tested now also had the content length header.

This comment has been minimized.

@arokem

arokem Mar 29, 2016

Member

Just to be sure: we now have a progress bar wherever that's possible ('content-length' is defined), and otherwise it just prints a "data-size is approximately X MB"?

This comment has been minimized.

@ghoshbishakh

ghoshbishakh Mar 29, 2016

Member

not exactly.
"data-size is approximately X MB" is printed whenever the size is defined even if 'content-length' is defined and progressbar is displayed.

I noticed some datasets in which 'content-length' and data-size both are not available.

@ghoshbishakh ghoshbishakh force-pushed the ghoshbishakh:fixProgressbar branch from 920fafb to 1f2f068 Mar 23, 2016

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 23, 2016

@arokem made the changes.

@arokem

This comment has been minimized.

Member

arokem commented Mar 23, 2016

Great. I don't have any more time to look today, but hopefully I will get
back to this tomorrow. In the meanwhile, we do also need to think about
this failure, if you get a chance:
https://travis-ci.org/arokem/dipy/jobs/118056298#L2279

On Wed, Mar 23, 2016 at 1:31 PM, Bishakh Ghosh notifications@github.com
wrote:

@arokem https://github.com/arokem made the changes.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1015 (comment)

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 23, 2016

Is it that tests are passing but build is failing?
It occured after the progressbar addition?

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 25, 2016

@arokem can you give me some idea when the errors are thrown as all the tests are successful in this PR

@arokem

This comment has been minimized.

Member

arokem commented Mar 25, 2016

Yes - the tests are all passing here, but that's because we are not running the dipy.data tests on Travis.

That's what #1012 is supposed to fix: when we install the data module's tests, we get test-failures happening (but only on Python 3, you can see that here: https://travis-ci.org/arokem/dipy/builds/118056296)

To debug this, you will have to run the tests locally on your machine, on Python 3.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 25, 2016

@arokem looks like this test provides a url that points to a file.

testfile_url = pathname2url(op.split(symmetric362)[0] + op.sep)

And that is not a HTTP url. So it cannot have HTTP headers. That is why we are getting this error on

response_size = opener.getheader("Content-Length")

@arokem

This comment has been minimized.

Member

arokem commented Mar 25, 2016

Yes. The test is written to mock up a network connection, even when we
don't have a network connection. As you can see, this works fine on Python
2. We might need to rethink how we mock up the network connection, though,
for Python 3.

On Fri, Mar 25, 2016 at 8:51 AM, Bishakh Ghosh notifications@github.com
wrote:

@arokem https://github.com/arokem looks like this test provides a url
that points to a file.

testfile_url = pathname2url(op.split(symmetric362)[0] + op.sep)

And that is not a HTTP url. So it cannot have HTTP headers. That is why we
are getting this error on

response_size = opener.getheader("Content-Length")


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1015 (comment)

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 25, 2016

I was studying urllib.request.urlopen and it actually returns different things for HTTP url and file url:

>>> from urllib.request import urlopen
>>> b = urlopen("http://www.google.com")
>>> a = urlopen("file:///home/bishakh/projects/dipy/dipy/data/files/evenly_distributed_sphere_362.npz")
>>> b
<http.client.HTTPResponse object at 0x7f9478d2e4a8>
>>> a
<addinfourl at 140275661532128 whose fp = <_io.BufferedReader name='/home/bishakh/projects/dipy/dipy/data/files/evenly_distributed_sphere_362.npz'>>
@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 25, 2016

@arokem The most straight forward solution is to fallback on error. I am trying to figure out something more elegant.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 27, 2016

@arokem should we enclose it in a try except to fallback then?

@arokem

This comment has been minimized.

Member

arokem commented Mar 28, 2016

It returns different things only on Python 3. One option is to start an http server locally (https://docs.python.org/3/library/http.server.html)and serve that file up (locally!) through that http server so that it looks just like it was served from a remote location.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 28, 2016

@arokem aah! So provided fetcher will never have to fetch a file with file uri we can change the test case that way! So should I proceed to change the test case then?

@arokem

This comment has been minimized.

Member

arokem commented Mar 28, 2016

Yes! The fetchers are always supposed to operate against HTTP URLs. The file URL was just a (too naive, apparently) attempt to simulate the real situation. If you can serve the file up through and HTTP server, that's better. Please do ahead and change the test case.

ghoshbishakh added some commits Mar 23, 2016

Fix progressbar of fetcher
Fallback to metadata download size when content-length is not defined in http headers
Fix fetcher tests
Previously file url was used to test the fetcher module.
To improve that and make it compatible with the progressbar in fetcher #971 use a local HTTP server to emulate HTTP downloads.
modified:   tests/test_fetcher.py

@ghoshbishakh ghoshbishakh force-pushed the ghoshbishakh:fixProgressbar branch from 1f2f068 to 09934d7 Mar 29, 2016

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 29, 2016

@arokem Looks better?
fetcher_tests are passing locally.

@arokem

This comment has been minimized.

Member

arokem commented Mar 29, 2016

Yes - this looks right to me. Nice work!

Let me rebase #1012 on top of this, so that we can see that it fixes that one.

@arokem

This comment has been minimized.

Member

arokem commented Mar 29, 2016

The rebase might get a bit complicated. I have a better idea: would you mind adding the following one-line change in the setup.py to this PR?

7ce3a6f

This addition will make sure that the tests also run on all of our bots.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 29, 2016

@arokem done! hoping for the best 😄

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 29, 2016

@arokem waaah! tests passed! 😄

@arokem

This comment has been minimized.

Member

arokem commented Mar 29, 2016


# stop local HTTP Server
server.shutdown()
# change to original working directory

This comment has been minimized.

@arokem

arokem Mar 29, 2016

Member

Do you want to stop the server_thread as well? I actually don't know whether that's needed, which is why I am asking.

This comment has been minimized.

@ghoshbishakh

ghoshbishakh Mar 29, 2016

Member

no actually force closing the thread is not necessary. server.shutdown() ends the server and the thread target function returns safely and the thread dies peacefully (based on what I understood about threads)

This comment has been minimized.

@arokem

arokem Mar 29, 2016

Member

Cool. Thanks!

@arokem

This comment has been minimized.

Member

arokem commented Mar 29, 2016

I've confirmed that this works well. One more tiny comment: could you put in a line-break after the progress bar? See here for example, how the "Files downloaded successfully" message comes in straight after the progress bar:

screen shot 2016-03-29 at 2 27 41 pm

@arokem

This comment has been minimized.

Member

arokem commented Mar 29, 2016

Also, is there a way to suppress these messages that appear in the terminal while running this in the jupyter notebook?

screen shot 2016-03-29 at 2 30 02 pm

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 30, 2016

I'll add the line break.
I could not understand themessages in thejupyter notebook. Can you tell me the process to test the issue?

@arokem

This comment has been minimized.

Member

arokem commented Mar 30, 2016

Try running this notebook: https://gist.github.com/arokem/fce1873952a0b329c1007b44ec2d0b4e

As the notebook runs, these messages appear in the terminal (in the background). It might not be a big deal, but if you can find an easy fix, that would be good.

Fix Progressbar
Fix jupyter stty size access error issue
@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 30, 2016

@arokem using tput cols to determine size of the terminal fixed the issue for me!

@arokem

This comment has been minimized.

Member

arokem commented Mar 30, 2016

Yep. That fixed it for me.

I am going to go ahead and merge this now, as it fixes an existing bug on master. If others have further things, please make a PR :-)

@arokem

This comment has been minimized.

Member

arokem commented Mar 30, 2016

Just waiting for Travis to be finished...

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 30, 2016

@arokem thanks 😊

@arokem arokem merged commit f621ff7 into nipy:master Mar 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arokem

This comment has been minimized.

Member

arokem commented Mar 30, 2016

Nice work!

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 30, 2016

My first proper patch to any proper open source project! Feeling awesome! 😄

Though the bug was introduced by me in the first place! 😛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment