global: refactor downloader code #118

ParthS007 · 2021-01-05T15:44:34Z

global: refactor downloader code
cli: add --download-lib option in download-files

tiborsimko · 2021-01-06T08:31:36Z

cernopendata_client/downloader_class.py

+
+
+class Downloader(object):
+    """Downloader class for managing pycurl and requests related utilities functions."""


If we go for the object-oriented way, what about bringing it further:

make Downloader class abstract, with common methods for showing progress etc, and with abstract method download() that will get implemented in each subclass

create DownloaderPycurl and DownloaderRequests classes implementing download() function using each concrete downloader

This will also leave space for creating new classes:

DownloaderXRootD class, using xrootdpyfs; this is currently enough. But later we could have another class using vanilla xrootd client (as opposed to xrootdpyfs).

Note: OT1H, the common Downloader() class could take care of both HTTP and XRootD protocols, if it makes sense to have some common functions, such as progress reporting or argument massaging. OTOH, we could also make two distinct class hierarchies, DownloaderHttp (with its DownloaderHttpPycurl, DownloaderHttpRequests subclasses) and DownloaderXrootd, which could make separation cleaner.

The option with making Downloader class abstract and implementing DownloaderPycurl and DownloaderRequests, DownloaderXRootD looks good to me.

I am not sure how the progress or any other methods will get implemented in Xrootd case so making distinct class hierarchies, DownloaderHttp (with its DownloaderHttpPycurl, DownloaderHttpRequests subclasses) and DownloaderXrootd also makes sense.

Bit confused here for choosing any of these. 😄

After MM discussion, proceeded with the second option.

tiborsimko · 2021-01-06T08:46:15Z

cernopendata_client/cli.py

@@ -243,6 +243,13 @@ def get_file_locations(server, recid, doi, title, protocol, expand, verbose):
        DOWNLOAD_RETRY_SLEEP
    ),
 )
+@click.option(
+    "--download-lib",
+    "dl_lib",


dl_lib is not very descriptive. What about keeping the same variable name as the CLI option name. This will make searchers etc easier.

--download-lib would be better spelled --download-library. But if we keep this option also for XRootD and any other future methods, what about simply calling it --downloader? Or perhaps --downloader-engine?

--downloader-engine sounds good to me.

tiborsimko · 2021-01-06T08:47:58Z

cernopendata_client/downloader_class.py

@@ -0,0 +1,115 @@
+# -*- coding: utf-8 -*-
+# This file is part of cernopendata-client.


Do we need a new file? The class is relatively small, and if we introduce subclasses, the code will be nicely separated in that way. So I guess it can stay in the same downloader.py file. It would be simpler and more consistent with the other files present in the cernopendata-client package.

I created the new file just to have the class separated to just show and have an idea myself how it will look like, will move it to downloader.py.

tiborsimko · 2021-01-11T09:43:47Z

cernopendata_client/downloader.py

+            c.close()
+
+
+class DownloaderXrootd:


The new XRootD downloader does not work locally for me, the troubles are already with checking local file existence before attempting the first download... Here's a simple recipe:

$ git clean -d -ff -x $ $ cernopendata-client download-files --recid 5500 --protocol xrootd ==> Downloading file 1 of 11 Traceback (most recent call last): File ".../cernopendata-client/bin/cernopendata-client", line 8, in <module> sys.exit(cernopendata_client()) File ".../cernopendata-client/lib/python3.9/site-packages/click/core.py", line 829, in __call__ return self.main(*args, **kwargs) File ".../cernopendata-client/lib/python3.9/site-packages/click/core.py", line 782, in main rv = self.invoke(ctx) File ".../cernopendata-client/lib/python3.9/site-packages/click/core.py", line 1259, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) File ".../cernopendata-client/lib/python3.9/site-packages/click/core.py", line 1066, in invoke return ctx.invoke(self.callback, **ctx.params) File ".../cernopendata-client/lib/python3.9/site-packages/click/core.py", line 610, in invoke return callback(*args, **kwargs) File ".../cernopendata-client/lib/python3.9/site-packages/cernopendata_client/cli.py", line 354, in download_files check_error( File ".../cernopendata-client/lib/python3.9/site-packages/cernopendata_client/downloader.py", line 211, in check_error "size": os.path.getsize(file_dest), File "/usr/lib64/python3.9/genericpath.py", line 50, in getsize return os.stat(filename).st_size FileNotFoundError: [Errno 2] No such file or directory: '5500/BuildFile.xml'

tiborsimko · 2021-01-11T09:47:26Z

tests/test_cli_download_files.py

@@ -123,6 +123,24 @@ def test_download_files_https_requests(mocker):
        os.remove(test_file)


+def test_download_files_download_engine(mocker):


I see troubles with Python-2.7 locally. The importlib-resources was installed but is not Python-2.7 friendly, since it uses f-strings. You need to pin some package versions in setup.py conditionally, so that they would not be installed for Python-2.7. Here is a test case to try out locally:

$ tox -e py27 ... File "/home/tibor/private/project/opendata/src/cernopendata-client/.eggs/importlib_resources-0.0.0-py2.7.egg/importlib_resources/readers.py", line 77 raise FileNotFoundError(f'{self} is not a file') ^ ... File "/home/tibor/private/project/opendata/src/cernopendata-client/.tox/py27/lib/python2.7/site-packages/pkg_resources/__init__.py", line 786, in resolve raise DistributionNotFound(req, requirers) pkg_resources.DistributionNotFound: The 'importlib-resources>=1.0' distribution was not found and is required by virtualenv ERROR: InvocationError for command /home/tibor/private/project/opendata/src/cernopendata-client/.tox/py27/bin/python setup.py test (exited with code 1)

I tested the $ tox -e py27 in a python2.7 env. It worked fine without warnings.
log.txt

Solved: this was observed with OS native python3-tox package. Using tox from PyPI fixes the troubles.

tiborsimko · 2021-01-11T09:52:47Z

cernopendata_client/cli.py

+    "download_engine",
+    default="requests",
+    type=click.Choice(["requests", "pycurl"]),
+    help="Library to be used in downloading files [requests,pycurl, xrootd] [default=requests]",


Please replace "Library by "Engine" here and elsewhere, since the option is called "engine".

tiborsimko · 2021-01-11T09:55:54Z

cernopendata_client/downloader.py

-                    msg="File {} is incomplete. Resuming download.".format(
-                        file_name,
+                    msg_type="error",
+                    msg="{} is not installed on system. Please install it".format(


Note that there is a traceback coming after the message. We should exit without showing traceback. Also, please put a full stop after the error message.

$ cernopendata-client download-files --recid 5500 --download-engine=pycurl ==> Downloading file 1 of 11 ==> ERROR: pycurl is not installed on system. Please install it Traceback (most recent call last): File ".../cernopendata-client/bin/cernopendata-client", line 8, in <module> sys.exit(cernopendata_client()) File ".../cernopendata-client/lib/python3.9/site-packages/click/core.py", line 829, in __call__ return self.main(*args, **kwargs) File ".../cernopendata-client/lib/python3.9/site-packages/click/core.py", line 782, in main rv = self.invoke(ctx) File ".../cernopendata-client/lib/python3.9/site-packages/click/core.py", line 1259, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) File ".../cernopendata-client/lib/python3.9/site-packages/click/core.py", line 1066, in invoke return ctx.invoke(self.callback, **ctx.params) File ".../cernopendata-client/lib/python3.9/site-packages/click/core.py", line 610, in invoke return callback(*args, **kwargs) File ".../cernopendata-client/lib/python3.9/site-packages/cernopendata_client/cli.py", line 348, in download_files download_single_file( File ".../cernopendata-client/lib/python3.9/site-packages/cernopendata_client/downloader.py", line 322, in download_single_file downloader.file_downloader() File ".../cernopendata-client/lib/python3.9/site-packages/cernopendata_client/downloader.py", line 132, in file_downloader c = pycurl.Curl() NameError: name 'pycurl' is not defined

Another observation on the same track: is protocol option being checked against download engine option?

$ cernopendata-client download-files --recid 5500 --protocol xrootd --download-engine requests ==> Downloading file 1 of 11

One would expect an error message saying that protocol xrootd and engine requests are not compatible.

Please add some checks and test cases for that.

tiborsimko · 2021-01-12T09:42:06Z

cernopendata_client/cli.py

@@ -243,6 +243,12 @@ def get_file_locations(server, recid, doi, title, protocol, expand, verbose):
        DOWNLOAD_RETRY_SLEEP
    ),
 )
+@click.option(
+    "--download-engine",


We are using download-engine but elsewhere in the code we have downloader-engine:

tests/test_cli_download_files.py 127: """Test download_files() command with downloader-engine option.""" 145: """Test download_files() command with downloader-engine option and wrong protocol.""" 157: """Test download_files() command with downloader-engine option and wrong protocol."""

Let's standardise this.

BTW what about the simplest version downloader so that people type --downloader pycurl? Seems good to go nicely with class names...

I was thinking about downloader initially but then I thought download-engine sounds to be more specific.

tiborsimko · 2021-01-12T09:44:17Z

cernopendata_client/cli.py

+    "--download-engine",
+    "download_engine",
+    type=click.Choice(["requests", "pycurl", "xrootdpyfs"]),
+    help="Engine to be used in downloading files [requests, pycurl, xrootdpyfs] [default=requests(for HTTP), xrootdpyfs(for XRootD)]",


Download engine to use when downloading files. The available values are 'requests', 'pycurl', 'xrootdpyfs'. [default=requests (for HTTP protocol), xrootdpyfs (for XRootD protocol)]

tiborsimko · 2021-01-12T09:46:22Z

docs/usage.rst

@@ -247,6 +247,22 @@ xrootd`` command-line option to use that protocol instead of HTTP/HTTPS:
      -> File: ./5500/mass4l_combine.png
    ==> Success!

+**Downloading engine**


*Select downloader engine*

to go better with the other subtitles that use the verb style such as "Filter by name..."

tiborsimko · 2021-01-12T09:46:55Z

docs/usage.rst

+
+You can specify the download engine with ``--download-engine`` option. 
+
+- ``requests`` and ``pycurl`` are supported download engine for **HTTP** protocol.


are two supported download engines for

(plural)

tiborsimko · 2021-01-12T09:47:07Z

docs/usage.rst

+You can specify the download engine with ``--download-engine`` option. 
+
+- ``requests`` and ``pycurl`` are supported download engine for **HTTP** protocol.
+- ``xrootdpyfs`` is supported download engine for **XRootD** protocol.


is the only supported

tiborsimko · 2021-01-12T09:47:25Z

docs/usage.rst

+      -> File: ./5500/BuildFile.xml
+      -> Progress: 0/0 KiB (100%)
+    ==> Success!
+


remove extra empty line

tiborsimko · 2021-01-12T09:50:36Z

cernopendata_client/downloader.py

+        if download_engine not in download_engine_protocol_http_map:
+            display_message(
+                msg_type="error",
+                msg="{} is not compatible with {} protocol. Please use requests/pycurl engine.".format(


"Please use requests/pycurl" could be misunderstood. Better say "Please use 'requests' or 'pycurl'."

BTW, in order to have this extensible, perhaps you can have a config.py variable which protocol supports which download engine options, and print the message dynamically from that config variable?

(E.g. this will come handy when we shall add vanilla 'xrdcp' as opposed to 'xrootdpyfs'.)

codecov-io · 2021-01-12T11:39:41Z

Codecov Report

Merging #118 (f4c687c) into master (fb70a46) will decrease coverage by 3.20%.
The diff coverage is 66.93%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
- Coverage   90.01%   86.81%   -3.21%     
==========================================
  Files          11       11              
  Lines         631      675      +44     
==========================================
+ Hits          568      586      +18     
- Misses         63       89      +26

Impacted Files	Coverage Δ
cernopendata_client/downloader.py	`68.42% <64.03%> (-9.51%)`	⬇️
cernopendata_client/cli.py	`96.91% <100.00%> (+0.11%)`	⬆️
cernopendata_client/config.py	`100.00% <100.00%> (ø)`

tiborsimko · 2021-01-12T15:35:45Z

setup.py

@@ -27,6 +27,7 @@
    "pytest>=2.8.0",
    'pytest-mock>=2.0,<3.0 ; python_version=="2.7"',
    'pytest-mock>=3.0 ; python_version>="3"',
+    "tox>=3.21.0",


Actually we cannot leave it like this, since tox -e py27 would like to install tox, as part of the run, and it seems to fail for me for py27... So let's leave it out for simplicity. Otherwise we would have to do the python_version dance, but it is probably not worth it.

Adds `--download-engine` option to select pycurl, requests or xrootdpyfs engines when downloading files.

tiborsimko reviewed Jan 6, 2021

View reviewed changes

ParthS007 changed the title ~~global: refactor downloader code for pycurl and requests~~ global: refactor downloader code Jan 11, 2021

tiborsimko reviewed Jan 11, 2021

View reviewed changes

tiborsimko reviewed Jan 12, 2021

View reviewed changes

ParthS007 requested a review from tiborsimko January 12, 2021 14:46

tiborsimko reviewed Jan 12, 2021

View reviewed changes

downloader: code refactoring

3b74b0f

Adds `--download-engine` option to select pycurl, requests or xrootdpyfs engines when downloading files.

ParthS007 requested a review from tiborsimko January 12, 2021 15:51

tiborsimko approved these changes Jan 12, 2021

View reviewed changes

tiborsimko merged commit 3b74b0f into cernopendata:master Jan 12, 2021

tiborsimko mentioned this pull request Jan 12, 2021

download-files: expose HTTP downloader choice (requests vs pycurl) #116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

global: refactor downloader code #118

global: refactor downloader code #118

ParthS007 commented Jan 5, 2021 •

edited

Loading

tiborsimko Jan 6, 2021

ParthS007 Jan 6, 2021

ParthS007 Jan 6, 2021

tiborsimko Jan 6, 2021

ParthS007 Jan 6, 2021

tiborsimko Jan 6, 2021

ParthS007 Jan 6, 2021 •

edited

Loading

tiborsimko Jan 11, 2021

tiborsimko Jan 11, 2021

ParthS007 Jan 12, 2021

tiborsimko Jan 12, 2021

tiborsimko Jan 11, 2021

tiborsimko Jan 11, 2021

tiborsimko Jan 11, 2021

tiborsimko Jan 12, 2021

ParthS007 Jan 12, 2021

tiborsimko Jan 12, 2021

tiborsimko Jan 12, 2021

tiborsimko Jan 12, 2021

tiborsimko Jan 12, 2021

tiborsimko Jan 12, 2021

tiborsimko Jan 12, 2021

tiborsimko Jan 12, 2021

codecov-io commented Jan 12, 2021 •

edited

Loading

tiborsimko Jan 12, 2021



		class Downloader(object):
		"""Downloader class for managing pycurl and requests related utilities functions."""

		@@ -0,0 +1,115 @@
		# -- coding: utf-8 --
		# This file is part of cernopendata-client.

		@@ -123,6 +123,24 @@ def test_download_files_https_requests(mocker):
		os.remove(test_file)


		def test_download_files_download_engine(mocker):


		You can specify the download engine with ``--download-engine`` option.

		- ``requests`` and ``pycurl`` are supported download engine for HTTP protocol.

global: refactor downloader code #118

global: refactor downloader code #118

Conversation

ParthS007 commented Jan 5, 2021 • edited Loading

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

ParthS007 Jan 6, 2021 • edited Loading

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

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

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

codecov-io commented Jan 12, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ParthS007 commented Jan 5, 2021 •

edited

Loading

ParthS007 Jan 6, 2021 •

edited

Loading

codecov-io commented Jan 12, 2021 •

edited

Loading