Repository: Duke-GCB/DukeDSClient

Issue:
python2 issues downloading
When downloading using python2 the following errors occur:

## multiprocessing.Pool doesn't work with `with`
```
  File "ddsc/core/download.py", line 221, in _download_files
    with multiprocessing.Pool(self.num_workers) as pool:
AttributeError: __exit__
```
Associated Code:
https://github.com/Duke-GCB/DukeDSClient/blob/397c28fd29ddfbb4c0e66d36ab34c7390ee9fb35/ddsc/core/download.py#L221

To fix we could manually close the pool.

## makedirs exist_ok parameter not supported
```
  File "ddsc/core/download.py", line 279, in _download_file
    os.makedirs(os.path.dirname(output_path), exist_ok=True)
TypeError: makedirs() got an unexpected keyword argument 'exist_ok'
```
Associated Code:
https://github.com/Duke-GCB/DukeDSClient/blob/397c28fd29ddfbb4c0e66d36ab34c7390ee9fb35/ddsc/core/download.py#L278

We could check if the directory exists first.

Discussion:

Patch:
diff --git a/ddsc/core/download.py b/ddsc/core/download.py
index bd97279..9bc1d31 100644
--- a/ddsc/core/download.py
+++ b/ddsc/core/download.py
@@ -218,13 +218,16 @@ class ProjectFileDownloader(object):
         self._show_downloaded_files_status()
 
     def _download_files(self):
-        with multiprocessing.Pool(self.num_workers) as pool:
+        pool = multiprocessing.Pool(self.num_workers)
+        try:
             for project_file in self._get_project_files():
                 self._download_file(pool, project_file)
                 while self._work_queue_is_full():
                     self._wait_for_and_retry_failed_downloads(pool)
             while self._work_queue_is_not_empty():
                 self._wait_for_and_retry_failed_downloads(pool)
+        finally:
+            pool.close()
 
     def _show_downloaded_files_status(self):
         print("\nVerifying contents of {} downloaded files using file hashes.".format(self.files_to_download))
@@ -275,7 +278,9 @@ class ProjectFileDownloader(object):
 
     def _download_file(self, pool, project_file):
         output_path = project_file.get_local_path(self.dest_directory)
-        os.makedirs(os.path.dirname(output_path), exist_ok=True)
+        output_path_parent = os.path.dirname(output_path)
+        if not os.path.exists(output_path_parent):
+            os.makedirs(output_path_parent)
         file_download_state = FileDownloadState(project_file, output_path, self.config)
         self._async_download_file(pool, file_download_state)
 
@@ -318,6 +323,7 @@ class ProjectFileDownloader(object):
             files_downloaded,
             self.files_to_download
         ))
+        sys.stdout.flush()
 
     def make_spinner_char(self, current_time):
         half_seconds = int(current_time)


Test patch:
diff --git a/ddsc/core/tests/test_download.py b/ddsc/core/tests/test_download.py
index 6600663..a6a7a96 100644
--- a/ddsc/core/tests/test_download.py
+++ b/ddsc/core/tests/test_download.py
@@ -5,7 +5,7 @@ from ddsc.core.download import FileHash, FileHashStatus, FileDownloadState, Proj
 from ddsc.core.pathfilter import PathFilter
 import queue
 import requests
-from mock.mock import patch, Mock, call, mock_open
+from mock.mock import patch, Mock, call, mock_open, ANY
 
 
 class TestFileHash(TestCase):
@@ -206,7 +206,7 @@ class TestProjectFileDownloader(TestCase):
         self.project.get_project_files_generator.return_value = [
             (mock_project_file, {DDS_TOTAL_HEADER: 1})
         ]
-        mock_pool = mock_multiprocessing.Pool.return_value.__enter__.return_value
+        mock_pool = mock_multiprocessing.Pool.return_value
         mock_apply_async = mock_pool.apply_async
         mock_apply_async.return_value.get.return_value.status.get_status_line.return_value = 'Hash Status Line'
 
@@ -229,7 +229,6 @@ class TestProjectFileDownloader(TestCase):
 
     @patch('ddsc.core.download.multiprocessing')
     def test_download_files(self, mock_multiprocessing):
-        mock_pool = mock_multiprocessing.Pool.return_value.__enter__.return_value
         project_file_downloader = ProjectFileDownloader(self.config, self.dest_directory, self.project,
                                                         path_filter=None)
         project_file_downloader.show_progress_bar = Mock()
@@ -262,14 +261,14 @@ class TestProjectFileDownloader(TestCase):
 
         # Expected behavior is to download file 1, wait for that to finish, download file 2, then wait for one more time
         manager.assert_has_calls([
-            call.download_file(mock_pool, 'project_file1_obj'),
+            call.download_file(ANY, 'project_file1_obj'),
             call.work_queue_is_full(),
-            call.wait_for_and_retry_failed_downloads(mock_pool),
+            call.wait_for_and_retry_failed_downloads(ANY),
             call.work_queue_is_full(),
-            call.download_file(mock_pool, 'project_file2_obj'),
+            call.download_file(ANY, 'project_file2_obj'),
             call.work_queue_is_full(),
             call.work_queue_is_not_empty(),
-            call.wait_for_and_retry_failed_downloads(mock_pool),
+            call.wait_for_and_retry_failed_downloads(ANY),
             call.work_queue_is_not_empty()
         ])
 
@@ -389,6 +388,7 @@ class TestProjectFileDownloader(TestCase):
     @patch('ddsc.core.download.os')
     @patch('ddsc.core.download.FileDownloadState')
     def test_download_file(self, mock_file_download_state, mock_os, mock_multiprocessing):
+        mock_os.path.exists.return_value = False
         mock_pool = Mock()
         mock_project_file = Mock(file_url={'host': 'somehost', 'url': 'someurl'})
         mock_project_file.get_local_path.return_value = '/tmp/data.out'
@@ -396,7 +396,8 @@ class TestProjectFileDownloader(TestCase):
                                                         path_filter=None)
         project_file_downloader._download_file(mock_pool, mock_project_file)
         mock_os.path.dirname.assert_called_with("/tmp/data.out")
-        mock_os.makedirs.assert_called_with(mock_os.path.dirname.return_value, exist_ok=True)
+        mock_os.path.exists.assert_called_with(mock_os.path.dirname.return_value)
+        mock_os.makedirs.assert_called_with(mock_os.path.dirname.return_value)
         mock_file_download_state.assert_called_with(mock_project_file, '/tmp/data.out', self.config)
         mock_pool.apply_async.assert_called_with(download_file, (mock_file_download_state.return_value,
                                                                  project_file_downloader.message_queue))
@@ -474,6 +475,7 @@ class TestProjectFileDownloader(TestCase):
         downloader.get_download_progress.return_value = (10, 1000)
         downloader.show_progress_bar()
         mock_sys.stdout.write.assert_called_with('\r| downloaded 1 KB @ 10 B/s          (10 of 20 files complete)')
+        mock_sys.stdout.flush.assert_called_with()
 
     @patch('ddsc.core.download.multiprocessing')
     def test_make_spinner_char(self, mock_multiprocessing):