Exp backoff for downloads. #9671

KellenSunderland · 2018-02-01T17:58:09Z

Description

The primary goal of this PR was to add exponential backoff to our http request. This should be a good feature for customers with slow Internet connections, but I also hope it will help mitigate test issues such as: #9669.

I noticed that we have a lot of duplicate code between test_utils.download and gluon.download. I've attempted to combine these two functions, but can roll it back if they had a good reason to be separated.

Checklist

Essentials

Passed code style checking (make lint)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Code is well-documented:

Note examples are affected by this change. I've updated them so that they do not reference download functions that no longer exist.

Comments

The tests require a new test dependency: requests_mock. Which may need to be added to the CI. Due to this, this PR may require a little iteration before test work properly. No rush to get it merged.
I've created a requirements.test.txt file for convenient installation of test deps.
NNVM will also need to be updated with a few new references in files tutorials/deploy_model_on_rasp.py and tutorials/from_mxnet.py.

szha · 2018-02-01T18:12:59Z

Looks like a breaking change

KellenSunderland · 2018-02-01T18:15:55Z

Yes, this should be considered a breaking API change. It removes the download function from gluon utils. Sorry, I should have called that out in the description.

szha · 2018-02-01T18:20:18Z

python/mxnet/utils.py

+
+
+def _download_retry_with_backoff(fname, url, max_attempts=4):
+    """Downloads a url with backoff applied, automatically re-requesting after a failure.


Retry can be added to existing download function.

This requires using a session right? I'm not sure if that then lets us stream to a file. I'll check it out though. I'd prefer to use built in functionality provided we get backoff and streaming support.

larroy

Nice change

larroy · 2018-02-01T18:09:15Z

python/mxnet/utils.py

+    backoff_coef = 50.0
+    while True:
+        try:
+            print('Downloading %s from %s...' % (fname, url))


I would use logging instead of print and arguments to logging instead of %

larroy · 2018-02-01T18:09:45Z

python/mxnet/utils.py

+            # Likely non-2** result, possibly timeout or redirection failure.
+            attempts = attempts + 1
+            if attempts > max_attempts:
+                print('Downloading %s from %s, failed after #%d attempts' % (fname, url, attempts))


same as above.

larroy · 2018-02-01T18:17:11Z

python/mxnet/utils.py

+
+    if overwrite or not os.path.exists(fname) or (sha1_hash and not check_sha1(fname, sha1_hash)):
+        dirname = os.path.dirname(os.path.abspath(os.path.expanduser(fname)))
+        if not os.path.exists(dirname):


you can use exist_ok and avoid the check:

https://docs.python.org/3/library/os.html#os.makedirs

that's py3 feature. https://docs.python.org/2.7/library/os.html#os.makedirs

larroy · 2018-02-01T18:21:53Z

python/mxnet/utils.py

+
+
+def create_dir(dirname):
+    if not os.path.exists(dirname):


see above about flag: https://docs.python.org/3/library/os.html#os.makedirs

szha

We can't have this change until 2.0. You may consider adding the retry to existing download functions.

KellenSunderland · 2018-02-01T18:31:41Z

@larroy: Thanks for the review, good tips.

@szha I'm alright with having this wait for 2.0. I can bring some of the non-breaking changes in with another minimal PR if you'd like.

szha · 2018-02-01T18:37:58Z

OK

piiswrong · 2018-02-01T18:48:40Z

python/mxnet/utils.py

+    return fname
+
+
+def create_dir(dirname):


if not os.path.exists(dirname):
os.makedirs(dirname)

This is just two lines. I don't think its worth making it a function

eric-haibin-lin · 2018-02-01T18:57:39Z

Can we keep the alias in test_utils and gluon to keep backward compatibility?

marcoabreu · 2018-02-01T23:30:07Z

python/mxnet/gluon/utils.py

@@ -61,13 +52,13 @@ def split_data(data, num_slice, batch_axis=0, even_split=True):
    size = data.shape[batch_axis]
    if size < num_slice:
        raise ValueError(
-            "Too many slices for data with shape %s. Arguments are " \
-            "num_slice=%d and batch_axis=%d."%(str(data.shape), num_slice, batch_axis))
+            "Too many slices for data with shape %s. Arguments are "


Isn't the backslash required?

Isn't triple quotes useful in this case? """blahs dah"""

I think triplequotes would be the equiv of

"Too many slices for data with shape %s. Arguments are \n" \ "num_slice=%d and batch_axis=%d."%(str(data.shape), num_slice, batch_axis))

but I'll try a few options.

marcoabreu · 2018-02-01T23:30:22Z

python/mxnet/gluon/utils.py

-            "data with shape %s cannot be evenly split into %d slices along axis %d. " \
-            "Use a batch size that's multiple of %d or set even_split=False to allow " \
-            "uneven partitioning of data."%(
+            "data with shape %s cannot be evenly split into %d slices along axis %d. "


Backslashes

KellenSunderland · 2018-02-04T15:12:28Z

@eric-haibin-lin Yes I was thinking of just pointing everything to the gluon download function for now.

CodingCat · 2018-03-07T04:43:34Z

Hi, the community has passed to vote about associating the code changes with JIRA (https://lists.apache.org/thread.html/ab22cf0e35f1bce2c3bf3bec2bc5b85a9583a3fe7fd56ba1bbade55f@%3Cdev.mxnet.apache.org%3E)

We have updated the guidelines for contributors in https://cwiki.apache.org/confluence/display/MXNET/Development+Process, please ensure that you have created a JIRA at https://issues.apache.org/jira/projects/MXNET/issues/ to describe your work in this pull request and include the JIRA title in your PR as [MXNET-xxxx] your title where MXNET-xxxx is the JIRA id

Thanks!

KellenSunderland · 2018-03-29T19:50:11Z

Closing, will reopen if I have time to refactor.

Pep fixes

34047f7

KellenSunderland requested a review from szha as a code owner February 1, 2018 17:58

KellenSunderland added 3 commits February 1, 2018 19:07

Add exp backoff for downloads, refactor utils

faf4f93

Add test for new download util

e49aed3

Add requirements so users can install test dependencies

7fb941a

KellenSunderland force-pushed the exp_backoff branch from b6d91c7 to 7fb941a Compare February 1, 2018 18:08

szha added the API change label Feb 1, 2018

szha reviewed Feb 1, 2018

View reviewed changes

larroy approved these changes Feb 1, 2018

View reviewed changes

szha suggested changes Feb 1, 2018

View reviewed changes

szha added the Breaking label Feb 1, 2018

piiswrong reviewed Feb 1, 2018

View reviewed changes

marcoabreu reviewed Feb 1, 2018

View reviewed changes

szha mentioned this pull request Feb 2, 2018

[Discussion] MXNet 2.0 Roadmap (was: APIs that might be a good idea to break in 2.0) #9686

Closed

KellenSunderland closed this Mar 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exp backoff for downloads. #9671

Exp backoff for downloads. #9671

KellenSunderland commented Feb 1, 2018 •

edited

Loading

szha commented Feb 1, 2018

KellenSunderland commented Feb 1, 2018

szha Feb 1, 2018

KellenSunderland Feb 1, 2018

larroy left a comment

larroy Feb 1, 2018

larroy Feb 1, 2018

larroy Feb 1, 2018

szha Feb 3, 2018

larroy Feb 1, 2018

szha left a comment

KellenSunderland commented Feb 1, 2018

szha commented Feb 1, 2018

piiswrong Feb 1, 2018

eric-haibin-lin commented Feb 1, 2018

marcoabreu Feb 1, 2018

larroy Feb 2, 2018

KellenSunderland Feb 2, 2018 •

edited

Loading

marcoabreu Feb 1, 2018

KellenSunderland commented Feb 4, 2018

CodingCat commented Mar 7, 2018

KellenSunderland commented Mar 29, 2018



		def _download_retry_with_backoff(fname, url, max_attempts=4):
		"""Downloads a url with backoff applied, automatically re-requesting after a failure.

Exp backoff for downloads. #9671

Exp backoff for downloads. #9671

Conversation

KellenSunderland commented Feb 1, 2018 • edited Loading

Description

Checklist

Essentials

Comments

szha commented Feb 1, 2018

KellenSunderland commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szha left a comment

Choose a reason for hiding this comment

KellenSunderland commented Feb 1, 2018

szha commented Feb 1, 2018

Choose a reason for hiding this comment

eric-haibin-lin commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KellenSunderland Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KellenSunderland commented Feb 4, 2018

CodingCat commented Mar 7, 2018

KellenSunderland commented Mar 29, 2018

KellenSunderland commented Feb 1, 2018 •

edited

Loading

KellenSunderland Feb 2, 2018 •

edited

Loading