Refactor CLI client by kislyuk · Pull Request #77 · HumanCellAtlas/dcp-cli

kislyuk · 2017-11-20T02:14:47Z

Sorry about the giant diff.

Refactor the library.

Introduce a new class, hca.util.SwaggerClient, that manages a
Swagger API connection client and dynamically generates the client
bindings and CLI parser for any Swagger API given to it. The only
user of this class is currently hca.dss.DSSClient.
The class takes only the Swagger definition URL as its
configuration, and caches the API definition in the user config
directory after the first run. This allows dynamic configuration of
API endpoints and other API parameters. Any higher level commands
that should be added on top of the API bindings can be defined as
methods in hca.util.SwaggerClient subclasses, and added to the
commands attribute.
In the new class, enable a new transport subroutine. It is a wrapper
around Requests, as before, but now it allows explicit, idiomatic
control over pagination, streaming, and authentication.
The new class enables better autodoc generation. Run make docs;
open _build/html/index.html to see the updated autodocs.
Remove code generation machinery.

Connects to HumanCellAtlas/data-store#468

Connects to HumanCellAtlas/data-store#531

Fixes #36, #56, #60

codecov-io · 2017-11-20T03:02:17Z

Codecov Report

Merging #77 into master will increase coverage by 11.38%.
The diff coverage is 87.84%.

@@             Coverage Diff             @@
##           master      #77       +/-   ##
===========================================
+ Coverage   76.46%   87.84%   +11.38%     
===========================================
  Files          37       27       -10     
  Lines        1266      897      -369     
===========================================
- Hits          968      788      -180     
+ Misses        298      109      -189

Impacted Files	Coverage Δ
hca/upload/cli/forget_command.py	`100% <ø> (ø)`	⬆️
hca/upload/cli/upload_command.py	`84.37% <ø> (ø)`	⬆️
hca/upload/cli/list_area_command.py	`100% <ø> (ø)`	⬆️
hca/__init__.py	`100% <100%> (ø)`	⬆️
hca/util/_docs.py	`100% <100%> (ø)`
hca/upload/upload_config.py	`94.28% <100%> (-0.16%)`	⬇️
hca/upload/cli/list_areas_command.py	`100% <100%> (ø)`	⬆️
hca/util/compat.py	`100% <100%> (ø)`
hca/upload/cli/select_command.py	`100% <100%> (ø)`	⬆️
hca/version.py	`100% <100%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3dbf5b7...feca6e4. Read the comment docs.

ttung · 2017-11-22T06:55:13Z

+            finally:
+                response.close()
+                session.stream = False
+        return {}


why this return value?

I think it's preferable if all commands return JSON objects on their standard output by default.

ttung · 2017-11-22T06:57:15Z

+        logger.info("Uploading %i files from %s to %s", len(files_to_upload), src_dir, staging_bucket)
+        file_uuids, uploaded_keys = upload_to_cloud(files_to_upload, staging_bucket=staging_bucket, replica=replica,
+                                                    from_cloud=False)
+        for file_handle in filter(lambda x: not isinstance(x, str), files_to_upload):


when is files_to_upload ever a string?

It used to be that this could be an S3 URL prefix, but with this refactor it's no longer possible to pass that in. Thanks for the catch, I've removed the filter.

ttung · 2017-11-22T08:03:20Z

@@ -0,0 +1,332 @@
+"""


This module contains the core of the entire CLI, yet has barebones documentation. I don't think this is in a state where other people would be able to come in and be able to work on this codebase.

Great point, sorry about that! I've added a docstring, please take a look.

The added documentation is a great step, but mostly for someone who wants to understand what this module does. It's not really that useful for understanding how it works.

If I were to come in and try to work on this, I'd probably have to drop print(..) statements everywhere to understand that.

Bento007

Are there testcases for search using the paginator?

Bento007 · 2017-11-22T18:33:19Z

+    """
+    Client for the Data Storage Service API.
+    """
+    UPLOAD_BACKOFF_FACTOR = 1.618


out of curiosity where does this magic number come from?

It's the golden ratio :)

It doesn't really matter, the exponential backoff factor for HTTP request backoffs can be any number not too far from 2.

(Also, all of this code is a refactoring of the existing upload/download code, so I'm not the original author.)

Also add a tempfile backport and the argcomplete tab completion utility.

It was meant to filter out S3 URLs, but we no longer handle those here.

kislyuk · 2017-11-30T19:46:52Z

@Bento007, I have added a test case for the paginator.

ttung · 2017-12-01T21:18:57Z

+        return os.path.join(self._user_config_home, self._name)
+
+
+_config = None


why not just:

_config = HCAConfig(__name__)

I don't think you get much from lazy-initializing it.

I do think it's beneficial to lazy-initialize it, to avoid import-time side effects that include reading a bunch of files and registering an atexit handler that will write a bunch of files.

ttung · 2017-12-02T03:48:18Z

+    def __init__(self, *args, **kwargs):
+        super(SwaggerAPIException, self).__init__(*args, **kwargs)
+
+        self.code = self.response.status_code


there's a standardized template for responses that comes as part of the json. https://github.com/HumanCellAtlas/data-store/blob/b9471df71eeba3074a43ca50caee5c0e144f0f15/dss-api.yml#L998

Added error response fields to exception.

ttung · 2017-12-04T04:14:41Z

+    parser = HCAArgumentParser(description=__doc__, formatter_class=argparse.RawTextHelpFormatter)
+    parser.add_argument("--version", action="version", version="%(prog)s {version}".format(version=__version__))
+    parser.add_argument("--log-level", default=get_config().get("log_level"),
+                        help=str([logging.getLevelName(i) for i in range(0, 60, 10)]),


is there really a need for NOTSET?

Great point, removed.

ttung · 2017-12-04T06:28:12Z

+containing the Swagger API definition that the client is providing an
+interface for. On first use, this API definition will be downloaded
+and saved into the user config directory (for example,
+/Users/Alice/.config/hca) with a name determined by the base64


is there an update strategy?

There is not yet an update strategy. I imagine an update strategy could involve re-downloading the Swagger API definition when the hca package version has changed with respect to what's recorded in the user-specific config file, or if the cached API definition file's mtime is older than a certain time cutoff. But I'd like to land that in a separate PR. Filed #79 to keep track of this.

ttung · 2017-12-04T06:37:30Z

@@ -0,0 +1,332 @@
+"""


The added documentation is a great step, but mostly for someone who wants to understand what this module does. It's not really that useful for understanding how it works.

If I were to come in and try to work on this, I'd probably have to drop print(..) statements everywhere to understand that.

ttung

This seems mostly fine, but see the comments. I'd consider improving the documentation of hca/util/__init__.py since it seems so central to everything.

sampierson · 2017-11-21T23:12:26Z

        list_area_parser = staging_subparsers.add_parser(
-            'list', description="List contents of currently selected upload area.")
+            'list', description=cls.__doc__, help=cls.__doc__)
        list_area_parser.set_defaults(func=ListAreaCommand)


hca upload list is currently broken. It just prints help. Is that because this needs to be changed to parser.set_defaults(entry_point= like you have done for the other parsers?

This should be fixed now. I forgot to update all the locations in the upload source tree to change func to entry_point. I'll add a CLI test to make sure this code path is asserted on.

sampierson · 2017-11-21T23:13:25Z

@@ -4,12 +4,15 @@


 class ListAreasCommand:


hca upload areas now prints some weird return value:

scripts/hca upload areas aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee <- selected deadbeef-dead-dead-dead-beeeeeeeeeef 0e2ea682-c9d0-49ad-8fa0-b9d8ab2f7418 340ff122-308c-47cd-afa2-f324ded875b6 "<hca.upload.cli.list_areas_command.ListAreasCommand instance at 0x10dcd9050>"

This should be fixed now. In the new CLI entry point dispatch convention, the return value of the call to entry_point is printed on stdout if it's a string or bytes, or printed as JSON if it's a mapping. The upload commands are using a class constructor call (__new__/__init__), which returns an instance of the command class, which is not something that should be printed. I special-cased this in the entry point dispatcher to ignore the return value in this case. Alternatively, we could change the Command classes to move the command handler logic from __init__ to __call__, and change the entry points to be instances of the Command class instead of the classes themselves.

sampierson · 2017-11-21T23:15:28Z

+
+class HCAConfig(tweak.Config):
+    default_config_file = os.path.join(os.path.dirname(__file__), "default_config.json")
+    @property


blank line above

sampierson · 2017-11-21T23:16:09Z

+
+
+_config = None
+def get_config():


2 blank lines before method

sampierson · 2017-11-21T23:19:12Z

-import os
+from __future__ import absolute_import, division, print_function, unicode_literals
+
+import os, sys, argparse, logging, json, datetime, traceback


I keep getting dinged in code reviews for not splitting my imports onto individual lines.
Seems like you're going in the opposite direction.

CLI updates in HumanCellAtlas/dcp-cli#77 and subsequent commits, hca PyPI release v3.0.5

kislyuk requested review from Bento007, sampierson and ttung November 20, 2017 02:14

ghost assigned kislyuk Nov 20, 2017

ghost added the code review label Nov 20, 2017

kislyuk force-pushed the akislyuk-client-refactor branch 7 times, most recently from 287dbd8 to c939f0b Compare November 20, 2017 16:16

ttung reviewed Nov 22, 2017

View reviewed changes

Bento007 requested changes Nov 22, 2017

View reviewed changes

kislyuk force-pushed the akislyuk-client-refactor branch 2 times, most recently from bf0da4f to 6f237b4 Compare November 29, 2017 21:27

kislyuk added 9 commits November 29, 2017 15:07

Refactor subparser help in hca upload

c4927f5

Add dependencies for in-memory code generation

b98b62a

Also add a tempfile backport and the argcomplete tab completion utility.

Begin migration to new config manager

f1c6191

Remove old codegen DSS CLI

8d22b87

Add util submodule

1cfb6c8

Add new in-memory codegen Swagger CLI

55adc05

Minor changes to Sphinx autodoc config

94315d6

Break out client method factory

2629f17

Add SwaggerClient docs

ada4ad9

kislyuk force-pushed the akislyuk-client-refactor branch from 6f237b4 to ada4ad9 Compare November 30, 2017 18:58

Remove vestigal filter on file handles

02698b7

It was meant to filter out S3 URLs, but we no longer handle those here.

Add a barebones test case for paginator

4095942

Factor method args processing into a subroutine

f5bbd86

ttung reviewed Dec 1, 2017

View reviewed changes

ttung reviewed Dec 2, 2017

View reviewed changes

ttung reviewed Dec 4, 2017

View reviewed changes

ttung approved these changes Dec 4, 2017

View reviewed changes

hca --log-level: Remove unneeded NOTSET choice

260886a

kislyuk mentioned this pull request Dec 4, 2017

Strategy to update/automatically re-fetch API definition #79

Closed

Bento007 approved these changes Dec 4, 2017

View reviewed changes

Add structured error response data to exception

44d08aa

kislyuk force-pushed the akislyuk-client-refactor branch from 8785095 to 44d08aa Compare December 4, 2017 19:23

Pass parameter optionality metadata to CLI parser

feca6e4

kislyuk merged commit 22f9189 into master Dec 4, 2017

ghost removed the code review label Dec 4, 2017

kislyuk deleted the akislyuk-client-refactor branch December 4, 2017 19:56

sampierson reviewed Dec 5, 2017

View reviewed changes

kislyuk added a commit to HumanCellAtlas/data-store that referenced this pull request Dec 7, 2017

Smoketest fixes to go along with the CLI updates (#720)

fcb0a67

CLI updates in HumanCellAtlas/dcp-cli#77 and subsequent commits, hca PyPI release v3.0.5

kislyuk mentioned this pull request Dec 12, 2017

Fix typevar passthrough to argparse with typing.Optional kwargs #83

Merged

		return os.path.join(self._user_config_home, self._name)


		_config = None

Conversation

kislyuk commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bento007 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kislyuk Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kislyuk commented Nov 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kislyuk Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kislyuk commented Nov 20, 2017 •

edited

Loading

codecov-io commented Nov 20, 2017 •

edited

Loading

kislyuk Nov 22, 2017 •

edited

Loading

kislyuk Dec 5, 2017 •

edited

Loading