From b7fc61d4d8cd0354db572405ea25109b76077a3e Mon Sep 17 00:00:00 2001 From: stu1130 Date: Thu, 13 Sep 2018 17:40:40 -0700 Subject: [PATCH 01/21] use rename trick to achieve atomic write but didn't support python2 and windows --- python/mxnet/gluon/utils.py | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index d5a14a6859a7..f74269c0b586 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -23,6 +23,7 @@ import os import hashlib +import uuid import warnings import collections import weakref @@ -231,7 +232,7 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ fname = os.path.join(path, url.split('/')[-1]) else: fname = path - assert retries >= 0, "Number of retries should be at least 0" + assert retries >= 0, 'Number of retries should be at least 0' if not verify_ssl: warnings.warn( @@ -242,23 +243,34 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ dirname = os.path.dirname(os.path.abspath(os.path.expanduser(fname))) if not os.path.exists(dirname): os.makedirs(dirname) - while retries+1 > 0: + while retries + 1 > 0: # Disable pyling too broad Exception # pylint: disable=W0703 try: - print('Downloading %s from %s...'%(fname, url)) + print('Downloading {} from {}...'.format(fname, url)) r = requests.get(url, stream=True, verify=verify_ssl) if r.status_code != 200: - raise RuntimeError("Failed downloading url %s"%url) - with open(fname, 'wb') as f: + raise RuntimeError('Failed downloading url {}'.format(url)) + # create uuid for temporary files + random_uuid = str(uuid.uuid4()) + with open('{}.{}'.format(fname, random_uuid), 'wb') as f: + # create uuid for temporary files for chunk in r.iter_content(chunk_size=1024): if chunk: # filter out keep-alive new chunks f.write(chunk) + # if the target file exists(created by other processes), + # delete the temporary file + if os.path.exists(fname): + os.remove('{}.{}'.format(fname, random_uuid)) + else: + # atmoic operation in the same file system + os.replace('{}.{}'.format(fname, random_uuid), fname) if sha1_hash and not check_sha1(fname, sha1_hash): - raise UserWarning('File {} is downloaded but the content hash does not match.'\ - ' The repo may be outdated or download may be incomplete. '\ - 'If the "repo_url" is overridden, consider switching to '\ - 'the default repo.'.format(fname)) + raise UserWarning( + 'File {} is downloaded but the content hash does not match.' + ' The repo may be outdated or download may be incomplete. ' + 'If the "repo_url" is overridden, consider switching to ' + 'the default repo.'.format(fname)) break except Exception as e: retries -= 1 From 657a921b8599a44c326d681d3559e4838ae5dde7 Mon Sep 17 00:00:00 2001 From: stu1130 Date: Thu, 13 Sep 2018 17:41:16 -0700 Subject: [PATCH 02/21] add test for multiprocess download --- tests/python/unittest/test_gluon_utils.py | 46 ++++++++++++++++++++--- 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/tests/python/unittest/test_gluon_utils.py b/tests/python/unittest/test_gluon_utils.py index 431852427f53..512bd691b35d 100644 --- a/tests/python/unittest/test_gluon_utils.py +++ b/tests/python/unittest/test_gluon_utils.py @@ -19,6 +19,9 @@ import os import tempfile import warnings +import glob +import shutil +import multiprocessing as mp try: from unittest import mock @@ -46,15 +49,45 @@ def test_download_retries(): @mock.patch( 'requests.get', - mock.Mock(side_effect= - lambda *args, **kwargs: MockResponse(200, 'MOCK CONTENT' * 100))) + mock.Mock(side_effect=lambda *args, **kwargs: MockResponse(200, 'MOCK CONTENT' * 100))) +def _download_successful(tmp): + """ internal use for testing download successfully """ + mx.gluon.utils.download( + "https://raw.githubusercontent.com/apache/incubator-mxnet/master/README.md", + path=tmp) + + def test_download_successful(): + """ test download with one process """ tmp = tempfile.mkdtemp() tmpfile = os.path.join(tmp, 'README.md') - mx.gluon.utils.download( - "https://raw.githubusercontent.com/apache/incubator-mxnet/master/README.md", - path=tmpfile) - assert os.path.getsize(tmpfile) > 100 + _download_successful(tmpfile) + assert os.path.getsize(tmpfile) > 100, os.path.getsize(tmpfile) + pattern = os.path.join(tmp, 'README.md*') + # check only one file we want left + assert len(glob.glob(pattern)) == 1, glob.glob(pattern) + # delete temp dir + shutil.rmtree(tmp) + + +def test_multiprocessing_download_successful(): + """ test download with multiprocessing """ + tmp = tempfile.mkdtemp() + tmpfile = os.path.join(tmp, 'README.md') + process_list = [] + # test it with 10 processes + for i in range(100): + process_list.append(mp.Process( + target=_download_successful, args=(tmpfile,))) + process_list[i].start() + for i in range(100): + process_list[i].join() + assert os.path.getsize(tmpfile) > 100, os.path.getsize(tmpfile) + # check only one file we want left + pattern = os.path.join(tmp, 'README.md*') + assert len(glob.glob(pattern)) == 1, glob.glob(pattern) + # delete temp dir + shutil.rmtree(tmp) @mock.patch( @@ -62,6 +95,7 @@ def test_download_successful(): mock.Mock( side_effect=lambda *args, **kwargs: MockResponse(200, 'MOCK CONTENT'))) def test_download_ssl_verify(): + """ test download verify_ssl parameter """ with warnings.catch_warnings(record=True) as warnings_: mx.gluon.utils.download( "https://mxnet.incubator.apache.org/index.html", verify_ssl=False) From 353221a88bd2fd0ab8ef0b51ea40a88be87e9b4a Mon Sep 17 00:00:00 2001 From: stu1130 Date: Fri, 14 Sep 2018 22:37:51 -0700 Subject: [PATCH 03/21] implement atomic_replace referred by https://github.com/untitaker/python-atomicwrites --- python/mxnet/gluon/utils.py | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index f74269c0b586..c752e683d847 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -22,6 +22,8 @@ 'check_sha1', 'download'] import os +import sys +import io import hashlib import uuid import warnings @@ -196,6 +198,35 @@ def check_sha1(filename, sha1_hash): return sha1.hexdigest() == sha1_hash +if sys.platform != 'win32': + # refer to https://github.com/untitaker/python-atomicwrites + def _replace_atomic(src, dst): + os.rename(src, dst) +else: + from ctypes import windll, WinError + + _MOVEFILE_REPLACE_EXISTING = 0x1 + _MOVEFILE_WRITE_THROUGH = 0x8 + _windows_default_flags = _MOVEFILE_WRITE_THROUGH + + text_type = unicode if sys.version_info[0] == 2 else str + + def _path_to_unicode(x): + if not isinstance(x, text_type): + return x.decode(sys.getfilesystemencoding()) + return x + + def _handle_errors(rv): + if not rv: + raise WinError() + + def _replace_atomic(src, dst): + _handle_errors(windll.kernel32.MoveFileExW( + _path_to_unicode(src), _path_to_unicode(dst), + _windows_default_flags | _MOVEFILE_REPLACE_EXISTING + )) + + def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ssl=True): """Download an given URL @@ -258,13 +289,13 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ for chunk in r.iter_content(chunk_size=1024): if chunk: # filter out keep-alive new chunks f.write(chunk) - # if the target file exists(created by other processes), + # if the target file exists(created by other processes), # delete the temporary file if os.path.exists(fname): os.remove('{}.{}'.format(fname, random_uuid)) else: # atmoic operation in the same file system - os.replace('{}.{}'.format(fname, random_uuid), fname) + _replace_atomic('{}.{}'.format(fname, random_uuid), fname) if sha1_hash and not check_sha1(fname, sha1_hash): raise UserWarning( 'File {} is downloaded but the content hash does not match.' From 32ff8cd3fddd6eb03064c96af58cf9c02e074a17 Mon Sep 17 00:00:00 2001 From: stu1130 Date: Fri, 14 Sep 2018 22:52:23 -0700 Subject: [PATCH 04/21] change the number of testing process to 10 --- tests/python/unittest/test_gluon_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/python/unittest/test_gluon_utils.py b/tests/python/unittest/test_gluon_utils.py index 512bd691b35d..20f1c8c549ad 100644 --- a/tests/python/unittest/test_gluon_utils.py +++ b/tests/python/unittest/test_gluon_utils.py @@ -76,11 +76,11 @@ def test_multiprocessing_download_successful(): tmpfile = os.path.join(tmp, 'README.md') process_list = [] # test it with 10 processes - for i in range(100): + for i in range(10): process_list.append(mp.Process( target=_download_successful, args=(tmpfile,))) process_list[i].start() - for i in range(100): + for i in range(10): process_list[i].join() assert os.path.getsize(tmpfile) > 100, os.path.getsize(tmpfile) # check only one file we want left From 7ded53624f53f1fde6e4507b5f43d190097e5fde Mon Sep 17 00:00:00 2001 From: stu1130 Date: Fri, 14 Sep 2018 23:00:53 -0700 Subject: [PATCH 05/21] add docstring and disable linter --- python/mxnet/gluon/utils.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index c752e683d847..fddd75168c87 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -23,7 +23,6 @@ import os import sys -import io import hashlib import uuid import warnings @@ -201,6 +200,7 @@ def check_sha1(filename, sha1_hash): if sys.platform != 'win32': # refer to https://github.com/untitaker/python-atomicwrites def _replace_atomic(src, dst): + """Implement atomic os.replace with linux and OSX. Internal Usa only""" os.rename(src, dst) else: from ctypes import windll, WinError @@ -209,18 +209,21 @@ def _replace_atomic(src, dst): _MOVEFILE_WRITE_THROUGH = 0x8 _windows_default_flags = _MOVEFILE_WRITE_THROUGH - text_type = unicode if sys.version_info[0] == 2 else str + text_type = unicode if sys.version_info[0] == 2 else str # noqa def _path_to_unicode(x): + """Handle text decoding. Internal use only""" if not isinstance(x, text_type): return x.decode(sys.getfilesystemencoding()) return x def _handle_errors(rv): + """Handle WinError. Internal use only""" if not rv: raise WinError() def _replace_atomic(src, dst): + """Implement atomic os.replace with windows. Internal use only""" _handle_errors(windll.kernel32.MoveFileExW( _path_to_unicode(src), _path_to_unicode(dst), _windows_default_flags | _MOVEFILE_REPLACE_EXISTING From b4c6b1f03867ba8f4e6e1a99badcc418deb853c3 Mon Sep 17 00:00:00 2001 From: stu1130 Date: Mon, 17 Sep 2018 17:06:14 -0700 Subject: [PATCH 06/21] half way to address some issue reviewer have --- python/mxnet/gluon/utils.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index fddd75168c87..ceb30ee5d5bc 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -197,11 +197,14 @@ def check_sha1(filename, sha1_hash): return sha1.hexdigest() == sha1_hash -if sys.platform != 'win32': +if not sys.platform.startswith('win32'): # refer to https://github.com/untitaker/python-atomicwrites def _replace_atomic(src, dst): - """Implement atomic os.replace with linux and OSX. Internal Usa only""" - os.rename(src, dst) + """Implement atomic os.replace with linux and OSX. Internal use only""" + try: + os.rename(src, dst) + except OSError: + raise OSError('os.rename failed. Might be wrong source path.') else: from ctypes import windll, WinError @@ -288,7 +291,6 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ # create uuid for temporary files random_uuid = str(uuid.uuid4()) with open('{}.{}'.format(fname, random_uuid), 'wb') as f: - # create uuid for temporary files for chunk in r.iter_content(chunk_size=1024): if chunk: # filter out keep-alive new chunks f.write(chunk) @@ -296,6 +298,8 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ # delete the temporary file if os.path.exists(fname): os.remove('{}.{}'.format(fname, random_uuid)) + warnings.UserWarning( + 'File {} exists in file system so the downloaded file is deleted'.format(fname)) else: # atmoic operation in the same file system _replace_atomic('{}.{}'.format(fname, random_uuid), fname) From 901ed344663e9a54f8dfe12100230d5b15d0afa3 Mon Sep 17 00:00:00 2001 From: stu1130 Date: Mon, 17 Sep 2018 23:25:07 -0700 Subject: [PATCH 07/21] use warning instead of raise UserWarn --- python/mxnet/gluon/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index ceb30ee5d5bc..bbc6b1f2068d 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -298,7 +298,7 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ # delete the temporary file if os.path.exists(fname): os.remove('{}.{}'.format(fname, random_uuid)) - warnings.UserWarning( + warnings.warn( 'File {} exists in file system so the downloaded file is deleted'.format(fname)) else: # atmoic operation in the same file system From 91f9ca96ce577c88391c3fb7dffa95d11262cf6e Mon Sep 17 00:00:00 2001 From: stu1130 Date: Tue, 18 Sep 2018 18:09:16 -0700 Subject: [PATCH 08/21] check for sha1 --- python/mxnet/gluon/utils.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index bbc6b1f2068d..f33ed8a0d26c 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -294,9 +294,10 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ for chunk in r.iter_content(chunk_size=1024): if chunk: # filter out keep-alive new chunks f.write(chunk) - # if the target file exists(created by other processes), + # if the target file exists(created by other processes) + # and have the same hash with target file # delete the temporary file - if os.path.exists(fname): + if os.path.exists(fname) and sha1_hash and check_sha1(fname, sha1_hash): os.remove('{}.{}'.format(fname, random_uuid)) warnings.warn( 'File {} exists in file system so the downloaded file is deleted'.format(fname)) From 9ab6341979769adf391c2dc0cca1ffa2ddef5280 Mon Sep 17 00:00:00 2001 From: stu1130 Date: Tue, 18 Sep 2018 20:27:14 -0700 Subject: [PATCH 09/21] Trigger CI From 1985726f5447c0d8e10ce11aa00a3232f6f602bd Mon Sep 17 00:00:00 2001 From: stu1130 Date: Tue, 18 Sep 2018 21:28:07 -0700 Subject: [PATCH 10/21] fix the logic of checking hash --- python/mxnet/gluon/utils.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index f33ed8a0d26c..f3d682bbed6a 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -297,13 +297,13 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ # if the target file exists(created by other processes) # and have the same hash with target file # delete the temporary file - if os.path.exists(fname) and sha1_hash and check_sha1(fname, sha1_hash): + if not os.path.exists(fname) or (sha1_hash and not check_sha1(fname, sha1_hash)): + # atmoic operation in the same file system + _replace_atomic('{}.{}'.format(fname, random_uuid), fname) + else: os.remove('{}.{}'.format(fname, random_uuid)) warnings.warn( 'File {} exists in file system so the downloaded file is deleted'.format(fname)) - else: - # atmoic operation in the same file system - _replace_atomic('{}.{}'.format(fname, random_uuid), fname) if sha1_hash and not check_sha1(fname, sha1_hash): raise UserWarning( 'File {} is downloaded but the content hash does not match.' From df2a9ef7d7e43092ebf7e842caf8cd70f3769df0 Mon Sep 17 00:00:00 2001 From: stu1130 Date: Thu, 20 Sep 2018 09:36:40 -0700 Subject: [PATCH 11/21] refine the error message --- python/mxnet/gluon/utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index f3d682bbed6a..746f974d5ada 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -204,7 +204,8 @@ def _replace_atomic(src, dst): try: os.rename(src, dst) except OSError: - raise OSError('os.rename failed. Might be wrong source path.') + raise OSError( + 'Moving downloaded temp file - {}, to {} failed. Please retry the download.'.format(src, dst)) else: from ctypes import windll, WinError From 3bed37cc0cd0b670f52e517405a8858015025600 Mon Sep 17 00:00:00 2001 From: stu1130 Date: Fri, 21 Sep 2018 11:19:54 -0700 Subject: [PATCH 12/21] add more comments and expose the error message to the user --- python/mxnet/gluon/utils.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index 746f974d5ada..a886d02469ed 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -207,9 +207,12 @@ def _replace_atomic(src, dst): raise OSError( 'Moving downloaded temp file - {}, to {} failed. Please retry the download.'.format(src, dst)) else: - from ctypes import windll, WinError + import ctypes _MOVEFILE_REPLACE_EXISTING = 0x1 + # Setting this value guarantees that a move performed as a copy + # and delete operation is flushed to disk before the function returns. + # The flush occurs at the end of the copy operation. _MOVEFILE_WRITE_THROUGH = 0x8 _windows_default_flags = _MOVEFILE_WRITE_THROUGH @@ -221,14 +224,22 @@ def _path_to_unicode(x): return x.decode(sys.getfilesystemencoding()) return x + def _format_error(err): + return ctypes.FormatError(_path_to_unicode(err)) + def _handle_errors(rv): - """Handle WinError. Internal use only""" + """Handle WinError. + Internal use only""" if not rv: - raise WinError() + msg = _format_error(ctypes.GetLastError()) + raise ctypes.WinError(msg) def _replace_atomic(src, dst): - """Implement atomic os.replace with windows. Internal use only""" - _handle_errors(windll.kernel32.MoveFileExW( + """Implement atomic os.replace with windows. + refer to https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-movefileexw + The function fails when one of the process(copy, flush, delete) fails. + Internal use only""" + _handle_errors(ctypes.windll.kernel32.MoveFileExW( _path_to_unicode(src), _path_to_unicode(dst), _windows_default_flags | _MOVEFILE_REPLACE_EXISTING )) From f0b8c5d9ae699cb7696e0f4de7ae64673e6d4eb0 Mon Sep 17 00:00:00 2001 From: stu1130 Date: Fri, 21 Sep 2018 11:20:52 -0700 Subject: [PATCH 13/21] delete trailing whitespace --- python/mxnet/gluon/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index a886d02469ed..a830ff98ef27 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -228,7 +228,7 @@ def _format_error(err): return ctypes.FormatError(_path_to_unicode(err)) def _handle_errors(rv): - """Handle WinError. + """Handle WinError. Internal use only""" if not rv: msg = _format_error(ctypes.GetLastError()) From 47f65c25fb9570f5ce7da6c2fc601f5b2d53c19e Mon Sep 17 00:00:00 2001 From: stu1130 Date: Fri, 21 Sep 2018 20:41:34 -0700 Subject: [PATCH 14/21] rename _path_to_encode to _str_to_unicode --- python/mxnet/gluon/utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index a830ff98ef27..004cf2b6a767 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -218,14 +218,14 @@ def _replace_atomic(src, dst): text_type = unicode if sys.version_info[0] == 2 else str # noqa - def _path_to_unicode(x): + def _str_to_unicode(x): """Handle text decoding. Internal use only""" if not isinstance(x, text_type): return x.decode(sys.getfilesystemencoding()) return x def _format_error(err): - return ctypes.FormatError(_path_to_unicode(err)) + return ctypes.FormatError(_str_to_unicode(err)) def _handle_errors(rv): """Handle WinError. @@ -240,7 +240,7 @@ def _replace_atomic(src, dst): The function fails when one of the process(copy, flush, delete) fails. Internal use only""" _handle_errors(ctypes.windll.kernel32.MoveFileExW( - _path_to_unicode(src), _path_to_unicode(dst), + _str_to_unicode(src), _str_to_unicode(dst), _windows_default_flags | _MOVEFILE_REPLACE_EXISTING )) From b4235f014f570a0c48cf0785fb1d0312ee2ae3da Mon Sep 17 00:00:00 2001 From: stu1130 Date: Mon, 1 Oct 2018 23:49:35 -0700 Subject: [PATCH 15/21] fix the error message bug and add remove when the movefile fail on windows --- python/mxnet/gluon/utils.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index 004cf2b6a767..76878af31ab8 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -224,15 +224,13 @@ def _str_to_unicode(x): return x.decode(sys.getfilesystemencoding()) return x - def _format_error(err): - return ctypes.FormatError(_str_to_unicode(err)) - - def _handle_errors(rv): - """Handle WinError. - Internal use only""" + def _handle_errors(rv, src): + """Handle WinError. Internal use only""" if not rv: - msg = _format_error(ctypes.GetLastError()) - raise ctypes.WinError(msg) + msg = ctypes.FormatError(ctypes.GetLastError()) + # if the MoveFileExW fail, remove the tempfile + os.remove(src) + raise OSError(msg) def _replace_atomic(src, dst): """Implement atomic os.replace with windows. @@ -242,7 +240,7 @@ def _replace_atomic(src, dst): _handle_errors(ctypes.windll.kernel32.MoveFileExW( _str_to_unicode(src), _str_to_unicode(dst), _windows_default_flags | _MOVEFILE_REPLACE_EXISTING - )) + ), src) def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ssl=True): @@ -328,8 +326,8 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ if retries <= 0: raise e else: - print("download failed, retrying, {} attempt{} left" - .format(retries, 's' if retries > 1 else '')) + print('download failed due to {}, retrying, {} attempt{} left' + .format(repr(e), retries, 's' if retries > 1 else '')) return fname From f0776b9ad27f3f6e2c9df0b0a1f9e93c2c5a961c Mon Sep 17 00:00:00 2001 From: stu1130 Date: Tue, 2 Oct 2018 09:52:23 -0700 Subject: [PATCH 16/21] add remove temp file for non-windows os --- python/mxnet/gluon/utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index 76878af31ab8..21e9e1f8d56a 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -204,6 +204,7 @@ def _replace_atomic(src, dst): try: os.rename(src, dst) except OSError: + os.remove(src) raise OSError( 'Moving downloaded temp file - {}, to {} failed. Please retry the download.'.format(src, dst)) else: From c7756dfe4c87dfee8382b74d2340a5016a8cdb62 Mon Sep 17 00:00:00 2001 From: stu1130 Date: Tue, 2 Oct 2018 10:01:41 -0700 Subject: [PATCH 17/21] handle the OSError caused by os.remove --- python/mxnet/gluon/utils.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index 21e9e1f8d56a..c4b02f605b3d 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -204,7 +204,10 @@ def _replace_atomic(src, dst): try: os.rename(src, dst) except OSError: - os.remove(src) + try: + os.remove(src) + except OSError: + pass raise OSError( 'Moving downloaded temp file - {}, to {} failed. Please retry the download.'.format(src, dst)) else: @@ -230,7 +233,10 @@ def _handle_errors(rv, src): if not rv: msg = ctypes.FormatError(ctypes.GetLastError()) # if the MoveFileExW fail, remove the tempfile - os.remove(src) + try: + os.remove(src) + except OSError: + pass raise OSError(msg) def _replace_atomic(src, dst): From 971ecf99181634e91d0636b3f41e2fdedccbebdb Mon Sep 17 00:00:00 2001 From: stu1130 Date: Tue, 2 Oct 2018 14:00:50 -0700 Subject: [PATCH 18/21] Trigger CI From af5b8ecb4dffe0cea6679cb82e4082f06d6395ef Mon Sep 17 00:00:00 2001 From: stu1130 Date: Mon, 8 Oct 2018 10:43:23 -0700 Subject: [PATCH 19/21] use finally to raise failure of atomic replace --- python/mxnet/gluon/utils.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index c4b02f605b3d..009bfac71b08 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -208,8 +208,10 @@ def _replace_atomic(src, dst): os.remove(src) except OSError: pass - raise OSError( - 'Moving downloaded temp file - {}, to {} failed. Please retry the download.'.format(src, dst)) + finally: + raise OSError( + 'Moving downloaded temp file - {}, to {} failed. \ + Please retry the download.'.format(src, dst)) else: import ctypes @@ -232,12 +234,13 @@ def _handle_errors(rv, src): """Handle WinError. Internal use only""" if not rv: msg = ctypes.FormatError(ctypes.GetLastError()) - # if the MoveFileExW fail, remove the tempfile + # if the MoveFileExW fails(e.g. fail to acquire file lock), removes the tempfile try: os.remove(src) except OSError: pass - raise OSError(msg) + finally: + raise OSError(msg) def _replace_atomic(src, dst): """Implement atomic os.replace with windows. From 6af8945ca0930f07be5e6c1065cffd8d7e3531f5 Mon Sep 17 00:00:00 2001 From: stu1130 Date: Mon, 8 Oct 2018 22:32:36 -0700 Subject: [PATCH 20/21] add missing try except block for os.remove --- python/mxnet/gluon/utils.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index 009bfac71b08..51ee720fc1c3 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -321,9 +321,13 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ # atmoic operation in the same file system _replace_atomic('{}.{}'.format(fname, random_uuid), fname) else: - os.remove('{}.{}'.format(fname, random_uuid)) - warnings.warn( - 'File {} exists in file system so the downloaded file is deleted'.format(fname)) + try: + os.remove('{}.{}'.format(fname, random_uuid)) + except OSError: + pass + finally: + warnings.warn( + 'File {} exists in file system so the downloaded file is deleted'.format(fname)) if sha1_hash and not check_sha1(fname, sha1_hash): raise UserWarning( 'File {} is downloaded but the content hash does not match.' From 676586211b6b0e37ea6e241553b7bc4bc026850b Mon Sep 17 00:00:00 2001 From: stu1130 Date: Wed, 10 Oct 2018 12:39:39 -0700 Subject: [PATCH 21/21] add retries value to error message --- python/mxnet/gluon/utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/mxnet/gluon/utils.py b/python/mxnet/gluon/utils.py index 51ee720fc1c3..78324986760a 100644 --- a/python/mxnet/gluon/utils.py +++ b/python/mxnet/gluon/utils.py @@ -289,7 +289,8 @@ def download(url, path=None, overwrite=False, sha1_hash=None, retries=5, verify_ fname = os.path.join(path, url.split('/')[-1]) else: fname = path - assert retries >= 0, 'Number of retries should be at least 0' + assert retries >= 0, "Number of retries should be at least 0, currently it's {}".format( + retries) if not verify_ssl: warnings.warn(