Add Batch data plane commands. #1875

xingwu1 · 2017-01-26T00:04:04Z

No description provided.

azurecla · 2017-01-26T00:05:24Z

Hi @xingwu1, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.azure.com.

TTYL, AZPRBOT;

troydai

The pull request is fairly big. Without sufficient knowledge of Azure Batch itself, I'm afraid I won't get the full picture of it. However, after went through the pull request, I do have a concern I hope to address.

The concern is that the batch command's loading performance is poorer comparing to other commands.

DEBUG: Loaded module 'acs' in 0.065 seconds.
DEBUG: Loaded module 'appservice' in 0.063 seconds.
DEBUG: Loaded module 'batch' in 0.191 seconds.
DEBUG: Loaded module 'cloud' in 0.000 seconds.
DEBUG: Loaded module 'component' in 0.001 seconds.
DEBUG: Loaded module 'configure' in 0.001 seconds.
DEBUG: Loaded module 'container' in 0.001 seconds.
DEBUG: Loaded module 'context' in 0.000 seconds.
DEBUG: Loaded module 'feedback' in 0.000 seconds.
DEBUG: Loaded module 'iot' in 0.012 seconds.
DEBUG: Loaded module 'keyvault' in 0.028 seconds.
DEBUG: Loaded module 'network' in 0.087 seconds.
DEBUG: Loaded module 'profile' in 0.001 seconds.
DEBUG: Loaded module 'redis' in 0.006 seconds.
DEBUG: Loaded module 'resource' in 0.003 seconds.
DEBUG: Loaded module 'role' in 0.006 seconds.
DEBUG: Loaded module 'sql' in 0.015 seconds.
DEBUG: Loaded module 'storage' in 0.032 seconds.
DEBUG: Loaded module 'taskhelp' in 0.001 seconds.
DEBUG: Loaded module 'vm' in 0.020 seconds.

You can find the data at the end of the CI log.

I suspect the bad performance is caused by the complex logic which generates commands. A lot of regular expressions are used, which I'm wondering if can be replaced by the usage of inspect module. Were the alternative solution considered?

In addition, a lot of the reflections (inspection) are based on the doc string. I'm not sure how reliable it is. If a documentation issue can escalate into runtime issue, I would say the design is too fragile.

/cc @derekbekoe

troydai · 2017-01-26T00:47:57Z

...mand_modules/azure-cli-batch/azure/cli/command_modules/batch/tests/data/batchUpdatePool.json

@@ -0,0 +1,9 @@
+{
+  "startTask": {
+    "commandLine": "cmd /c echo updated",


This won't work in shell (non-Windows). Will this cause any problem?

No, this is just a string to the test case, doesn't mean anything.

troydai · 2017-01-26T00:49:50Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/custom.py

+    except (ValidationError, ClientRequestError) as ex:
+        raise CLIError(ex)
+
+resize_pool.__doc__ = PoolResizeParameter.__doc__


You can use this decroator on resize_pool

I couldn't figure out how to use double decroator wrappers (need pass the parameter to decrator) and keep the original function signature. Could you give me a hint or example?

Sure. Here's an example

Python decorator is defined in PEP 318

troydai · 2017-01-26T00:57:40Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/custom.py

+        raise CLIError(ex)
+
+
+def resize_pool(client, pool_id, target_dedicated=None, #pylint:disable=too-many-arguments


The trailing comments here is not PEP8 compliant.

two spaces between the last char and #.

once space between the # and following char.

It is not a major issue. Just a reminding, once PEP8 check is turned on for this command module errors will show up.

troydai · 2017-01-26T01:01:53Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/commands.py

-from ._client_factory import batch_client_factory
+from ._client_factory import batch_client_factory, batch_data_service_factory
+from ._command_type import cli_data_plane_command
+from ._validators import validate_pool_settings


It is recommended to use full path instead of relative path. We do have a lot of relative path import in existing code base. I think they're bad practice, too.

troydai · 2017-01-26T01:13:50Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_command_type.py

+# TODO: Enum choice lists
+
+
+_CLASS_NAME = re.compile(r"<(.*?)>")  # Strip model name from class docstring


Is there a way to avoid regular expression?

investigating.

troydai · 2017-01-26T04:03:46Z

Note, if you want to check all PEP8 violation just run:

check_style --pep8 --module batch

troydai · 2017-01-26T05:32:32Z

src/azure-cli-core/azure/cli/core/decorators.py

@@ -95,8 +95,10 @@ def _wrapped_func(*args, **kwargs):
    return _decorator


-def transfer_doc(source_func):
+def transfer_doc(source_func, *additional_source_funcs):


The additional use case is a rare use case. Concatenating docstring is not use case in any other command module anytime soon. Therefore it is not ideal to modify the decorator just to fit a corner case. Please revoke the change and write a customized decorator in your own module.

troydai · 2017-01-26T05:33:24Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/custom.py

@@ -265,8 +262,6 @@ def update_pool(client, pool_id, json_file=None, command_line=None,  # pylint:di
    except (ValidationError, ClientRequestError) as ex:
        raise CLIError(ex)

-update_pool.__doc__ = PoolUpdatePropertiesParameter.__doc__ + "\n" + StartTask.__doc__


Out of curiosity, why concatenate two docstrings? Isn't the format of the new doc string broken?

No one will intend to directly call the this custom function. The docstring here is only used for build up the cli param help string. So the broken doc string doesn't bother any real user.

annatisch · 2017-01-26T18:37:56Z

@troydai,
The complex logic is almost entirely executed only on the specific command that is being run. Only one regex is executed in loading the module - and that's a precompiled one.
Unless the arguments for every command are loaded when the module is loaded? I was assuming the argument loader function was only called one that specific command had been executed?

troydai · 2017-01-26T18:44:43Z

@annatisch thanks for the clarification. I'm glad to know that. However, I think you should probe what hit the performance during the loading. The batch command is two times slower than the second slowest command module in the list. It absolute time will only increase in the user environment.

annatisch · 2017-01-26T18:46:13Z

@troydai,
That's for confirming that - yes I've been running it under the VS profiler, though having difficulties finding the culprit. Still investigating...

annatisch · 2017-01-26T19:00:51Z

@troydai, additionally - do you know what cmd I can use to specifically run and display those loading stats?

derekbekoe · 2017-01-26T19:01:16Z

@annatisch az --debug

annatisch · 2017-01-26T19:07:08Z

@troydai - thanks, I just figured that out ;)

derekbekoe

Is all the introspection in src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_command_type.py needed?

As @troydai pointed out, keep an eye on the time it takes to load the module with ‘az —debug’.
Right now, it’s taking a lot longer than others and this affects perf..

derekbekoe · 2017-01-26T18:32:03Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_client_factory.py

@@ -5,7 +5,20 @@

 from azure.mgmt.batch import BatchManagementClient

+import azure.batch.batch_service_client as batch
+import azure.batch.batch_auth as batchauth


In client factories, imports of SDK should go inside the factory methods themselves.
This is why the azure-cli-batch package takes so long to load.
The CLI delays calling the factory methods until a command that needs it is about to be executed.

e.g. on my core i7 macbook:

$ time python -c "from azure.mgmt.batch import BatchManagementClient" real 0m0.317s user 0m0.250s sys 0m0.064s

$ time python -c "import azure.batch.batch_auth as batchauth" real 0m0.515s user 0m0.354s sys 0m0.111s

FYI: @tjprescott, @troydai, @yugangw-msft, @johanste

Oh good catch - thanks!
Seems like imports are one of our major loading perf issues at this point

derekbekoe · 2017-01-26T18:41:33Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_params.py

+for command in ['list', 'show', 'create', 'set', 'delete', 'package']:
+    register_cli_argument('batch application {}'.format(command), 'account_name', batch_name_type, options_list=('--name', '-n'), validator=application_enabled)
+
+register_cli_argument('batch pool resize', 'if_modified_since', help='Specify this header to perform the operation only if the resource has been modified since the specified date/time.', type=datetime_format, arg_group='Pre-condition')


Would suggest removing Specify this header to from the help text.
Users of the command specify arguments not headers. This feels more like SDK documentation.

@derekbekoe - yeah these strings were copied straight from our swagger spec - we're endevouring to clean up this kind of terminology both here and in the spec itself :)

derekbekoe · 2017-01-26T18:43:33Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_params.py

+for item in ['batch certificate delete', 'batch certificate create', 'batch pool resize', 'batch pool reset', 'batch job list', 'batch task create']:
+    register_extra_cli_argument(item, 'account_name', arg_group='Batch Account',
+                                validator=validate_client_parameters,
+                                help='Batch account name. Environment variable: AZURE_BATCH_ACCOUNT')


What does Environment variable: AZURE_BATCH_ACCOUNT' mean?

@derekbekoe - These three environment variables are for authentication with our data plan API and are named as such to maintain consistency with the Xplat CLI.

Consider using az_config, the CLI configuration feature for this.

This would allow the batch account to be set with AZURE_BATCH_ACCOUNT (as required) but also allow setting the config in the CLI configuration file.

For example https://github.com/Azure/azure-cli/blob/master/src/command_modules/azure-cli-container/azure/cli/command_modules/container/custom.py#L23.

Another example https://github.com/Azure/azure-cli/blob/master/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/_validators.py#L51.

Already use az_config, if you look _validator.validate_client_parameters function.

derekbekoe · 2017-01-26T18:45:08Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_validators.py

+try:
+    from urllib.parse import urlsplit
+except ImportError:
+    from urlparse import urlsplit  # pylint: disable=import-error


from six.moves.urllib.parse import urlsplit instead.
https://pythonhosted.org/six/#module-six.moves.urllib.parse

derekbekoe · 2017-01-26T18:46:40Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/commands.py

-from ._client_factory import batch_client_factory
+from azure.cli.command_modules.batch._client_factory import (
+    batch_client_factory, batch_data_service_factory)
+from azure.cli.command_modules.batch._command_type import cli_data_plane_command


_command_type has a lot of code in it including several uses of regex.
All of that is executed before importing cli_data_plane_command.

derekbekoe · 2017-01-26T19:02:20Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/custom.py

+        raise CLIError(ex)
+
+
+def delete_certificate(client, thumbprint, thumbprint_algorithm, abort=None):


Since in here, the action is store_true, abort=False makes more sense IMHO.

derekbekoe · 2017-01-26T19:11:56Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/custom.py

+        except AttributeError:
+            raise CLIError(ex)
+    except (ValidationError, ClientRequestError) as ex:
+        raise CLIError(ex)


This try/except logic is used multiple times.
Consider moving out to helper function that takes a callable.
e.g.:

def delete_certificate(...): def handler(): if abort: client.cancel_deletion(thumbprint_algorithm, thumbprint) else: client.delete(thumbprint_algorithm, thumbprint) _ex_handle_patch_exceptions(handler) def _ex_handle_patch_exceptions(some_handler): try: some_handler() except BatchErrorException ... ...

Something like that.

derekbekoe · 2017-01-26T19:16:09Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/custom.py

+                                          if_unmodified_since=if_unmodified_since)
+
+    try:
+        return client.resize(pool_id, param, pool_resize_options=resize_option)


I question this convention that I've seen in this PR of having an abort flag that executes a different operation.
Should consider having these as two separate commands.

Does client.stop_resize return the same object structure as client.resize?
If not, this is another reason why you may want these as two separate commands.

derekbekoe · 2017-01-26T19:17:24Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/custom.py

+
+
+@transfer_doc(PoolUpdatePropertiesParameter, StartTask)
+def update_pool(client, pool_id, json_file=None, command_line=None,  # pylint:disable=too-many-arguments, W0613


Replace W0613 with the appropriate short-message.
This link should help http://pylint-messages.wikidot.com/all-messages.

Same with other instances.

derekbekoe · 2017-01-26T19:22:46Z

...es/azure-cli-batch/azure/cli/command_modules/batch/tests/data/batch-pool-create-invalid.json

+    "osFamily": "4",
+    "targetOSVersion": "*"
+  }
+  "targetDedicated": 2,


This character is highlighted in red on GitHub which makes me suspicious.
Can you verify that it's a : (colon)?

Missing a , following previous entity (on line 7).

@troydai, @derekbekoe,
This is a deliberately incorrect JSON file to test for bad input handling. Hence the file name suffix "invalid.json" :)

tjprescott

Overall, LGTM. A few questions on some things. I'll be interested to try out your logic for exposing complex types and seeing if we can eventually move that up into the CLI core.

tjprescott · 2017-01-26T21:32:47Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_client_factory.py

+    account_name = kwargs.pop('account_name', None)
+    account_key = kwargs.pop('account_key', None)
+    account_endpoint = kwargs.pop('account_endpoint', None)
+    kwargs.pop('force', None)


What is this for?

tjprescott · 2017-01-26T21:38:29Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_command_type.py

+        self.done = True
+
+
+class AzureDataPlaneCommand(object):


This should probably be AzureBatchDataPlaneCommand

tjprescott · 2017-01-26T21:40:37Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_command_type.py

+                                             help=docstring))
+
+
+def cli_data_plane_command(name, operation, client_factory, transform=None,  # pylint:disable=too-many-arguments


This should be cli_batch_data_plane_command.

tjprescott · 2017-01-26T21:46:12Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_params.py

+register_cli_argument('batch certificate', 'thumbprint', help='The certificate thumbprint.')
+register_cli_argument('batch certificate', 'thumbprint_algorithm', help='The certificate thumbprint algorithm.')
+register_cli_argument('batch certificate', 'password', help='The password to access the certificate\'s private key.')
+register_cli_argument('batch certificate', 'cert_file', type=file_type, help='The certificate file: cer file or pfx file.', validator=validate_cert_file, completer=FilesCompleter())


You should also specify type=file_type here and import file_type from azure.cli.core.commands.parameters. This ensures the leading ~ is expanded correctly.

It has already set the type to file_type.

tjprescott · 2017-01-26T21:46:51Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_params.py

+register_cli_argument('batch certificate', 'cert_file', type=file_type, help='The certificate file: cer file or pfx file.', validator=validate_cert_file, completer=FilesCompleter())
+register_cli_argument('batch certificate delete', 'abort', action='store_true', help='Cancel the failed certificate deletion operation.')
+
+register_cli_argument('batch task create', 'json_file', type=file_type, help='The file containing the task(s) to create in JSON format, if this parameter is specified, all other parameters are ignored.', validator=validate_json_file, completer=FilesCompleter())


Same here. type=file_type. Also, an alias to --task-file might be more intuitive than "--json-file".

It already be file_type type. I will udpate the name.

tjprescott · 2017-01-26T21:48:15Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_params.py

+                                help='Batch account name. Environment variable: AZURE_BATCH_ACCOUNT')
+    register_extra_cli_argument(item, 'account_key', arg_group='Batch Account',
+                                help='Batch account key. Must be used in conjunction with Batch account name and endpoint. Environment variable: AZURE_BATCH_ACCESS_KEY')
+    register_extra_cli_argument(item, 'account_endpoint', arg_group='Batch Account',


I assumed you added these three arguments to the Azure{Batch}DataPlaneCommand object to avoid having to do this. Why do you do both?

The AzureDataPlaneCommand is automatic create the parameter, and here is manually added to the only 5 custom commands.

xingwu1 · 2017-01-27T09:13:19Z

@troydai @derekbekoe

The latest result show the loading performance has been improved a lot

DEBUG: Loaded module 'batch' in 0.056 seconds

Consider the batch service has around 90 commands, it is quite good result.

tjprescott · 2017-01-27T16:06:33Z

Also, one thing we require of all New Command PRs is a dump of the -h help text. This helps us spot things like missing or illogical parameter help. Not every command is necessary--generally just the create commands.

Please add those as a comment on the PR (and omit the global arguments please).

annatisch · 2017-01-27T16:56:54Z

@tjprescott,
I've generated the help text into a separate branch on my fork:
Summary Commands (includes all create commands, both custom and 'auto parsed' and a couple of others, no global params)
Full Commands (includes all commands, group headings and globals parameters)

tjprescott · 2017-01-27T18:00:42Z

@annatisch is the help text updated? I started looking at it and immediately saw:

    --storage-account-id          : The resource id of the storage account to be used for auto

                                    storage account.

    --storage-account-name        : The storage account name to be used for auto storage account.

This is not consistent with the rest of the CLI. It would be called storage_account which would accept a name or id. But in the custom command it only looks like storage_account_id.

I just want to make sure before I start commenting on the help text that it's the latest.

annatisch · 2017-01-27T18:06:24Z

@tjprescott,
It was generated yesterday - so while it's not 100% latest, it's pretty recent.
Also - the command arguments you mentioned there are actually from the management module which has already been merged into master. But we can update it :)

FYI @xingwu1

xingwu1 · 2017-01-27T19:46:26Z

@tjprescott,
I change the code to use single storage account to accept both name and id scenario.

* node-user -> node user * Review feedback * More feedback fixes * pep fixes * fixed underscore * More touch ups

* Command refinements * More command feedback * Test fixes * More tests

annatisch · 2017-02-02T22:36:27Z

@tjprescott, @troydai

We've just finished an internal review of the commands and help documentation and I have updated the outputs here:
Summary Commands (includes all create commands, both custom and 'auto parsed' and a couple of others, no global params)
Full Commands (includes all commands, group headings and globals parameters)

The commands have reduced quite significantly - though still quite numerous. Let us know what you think. We've addressed the enum options (and removed thumbprint-algorithm), and prefixed more commands for clarification. We've reworded quite a lot of the help text to be more informative (e.g. correct use of Boolean flags).

FYI @itowlson

* streamlined commands * Updated tests * Fixed arg loading test

xingwu1 · 2017-02-06T20:04:44Z

@tjprescott
We reduced more parameters and add the VM image completer.

tjprescott

LGTM. Additional tweaks can still be make in follow-up PRs if needed.

xingwu1 · 2017-02-06T22:14:15Z

@troydai Could you review the change and sign off it if you feel good with the change?

* support silent args * Fixed pool error message * Further command editing * last fixes

annatisch · 2017-02-07T19:17:47Z

@troydai - would be great if you could take a look at the changes so we can get this merged today :)

troydai · 2017-02-07T22:17:10Z

I'm taking another look.

troydai · 2017-02-07T22:22:56Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_validators.py

+    except DeserializationError:
+        message = "Argument {} is not a valid ISO-8601 datetime format"
+        raise ValueError(message.format(value))
+    else:


not sure if this else is necessary here.

troydai · 2017-02-07T22:23:54Z

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_validators.py

+def validate_pool_resize_parameters(namespace):
+    """Validate pool resize parameters correct"""
+    if not namespace.abort:
+        if not namespace.target_dedicated:


if not namespace.abort and not namespace.target_dedicated:

johanste · 2017-02-07T23:35:30Z

Thanks everybody! Great to see this merged!

annatisch · 2017-02-07T23:37:31Z

Thanks guys!!

xingwu1 · 2017-02-07T23:38:41Z

Thanks!!!

azurecla added the cla-required label Jan 26, 2017

xingwu1 force-pushed the master branch 3 times, most recently from ba902d4 to 8a0029d Compare January 26, 2017 00:31

troydai reviewed Jan 26, 2017

View reviewed changes

troydai suggested changes Jan 26, 2017

View reviewed changes

xingwu1 force-pushed the master branch from d9e8567 to f8f7cd4 Compare January 26, 2017 05:57

annatisch requested a review from lmazuel January 26, 2017 16:14

derekbekoe reviewed Jan 26, 2017

View reviewed changes

tjprescott reviewed Jan 26, 2017

View reviewed changes

xingwu1 force-pushed the master branch 3 times, most recently from 5d85241 to 824eefb Compare January 27, 2017 08:51

tjprescott added New Command(s) Batch az batch labels Jan 27, 2017

xingwu1 force-pushed the master branch from fecf6d7 to b996f2a Compare January 27, 2017 20:24

xingwu1 and others added 4 commits February 2, 2017 12:25

PyLint and PEP8 clean up.

4396490

Use single parameter for storage account id and name.

1e50009

Review feedback + help fixes (#10)

0e02e1e

* node-user -> node user * Review feedback * More feedback fixes * pep fixes * fixed underscore * More touch ups

Review fixes (#11)

2186a7f

* Command refinements * More command feedback * Test fixes * More tests

xingwu1 force-pushed the master branch 3 times, most recently from 9a3f088 to d619798 Compare February 2, 2017 21:44

Add enum list for vmsize and thumbprint algorithm.

a6a510c

xingwu1 force-pushed the master branch from d619798 to a6a510c Compare February 2, 2017 22:05

Streamlined commands (#13)

275e9fe

* streamlined commands * Updated tests * Fixed arg loading test

xingwu1 force-pushed the master branch from aeb5284 to 275e9fe Compare February 6, 2017 20:03

tjprescott approved these changes Feb 6, 2017

View reviewed changes

annatisch added 2 commits February 6, 2017 14:52

Further command refinements (#14)

319bbf6

* support silent args * Fixed pool error message * Further command editing * last fixes

Fixed tests (#15)

93929c6

xingwu1 force-pushed the master branch from d5660a5 to 93929c6 Compare February 7, 2017 00:53

troydai approved these changes Feb 7, 2017

View reviewed changes

derekbekoe added cla-not-required and removed cla-required labels Feb 7, 2017

Change the coding style.

1947a80

derekbekoe merged commit ba47219 into Azure:master Feb 7, 2017

haroldrandom added the cla-required label Oct 25, 2019

		raise CLIError(ex)


		def resize_pool(client, pool_id, target_dedicated=None, #pylint:disable=too-many-arguments

		# TODO: Enum choice lists


		_CLASS_NAME = re.compile(r"<(.*?)>") # Strip model name from class docstring

		raise CLIError(ex)


		def delete_certificate(client, thumbprint, thumbprint_algorithm, abort=None):



		@transfer_doc(PoolUpdatePropertiesParameter, StartTask)
		def update_pool(client, pool_id, json_file=None, command_line=None, # pylint:disable=too-many-arguments, W0613

		help=docstring))


		def cli_data_plane_command(name, operation, client_factory, transform=None, # pylint:disable=too-many-arguments

Add Batch data plane commands. #1875

Add Batch data plane commands. #1875

Conversation

xingwu1 commented Jan 26, 2017

azurecla commented Jan 26, 2017

troydai left a comment • edited

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

troydai commented Jan 26, 2017

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

annatisch commented Jan 26, 2017

troydai commented Jan 26, 2017

annatisch commented Jan 26, 2017

annatisch commented Jan 26, 2017 • edited

derekbekoe commented Jan 26, 2017

annatisch commented Jan 26, 2017

derekbekoe 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

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

Choose a reason for hiding this comment

tjprescott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott Jan 26, 2017 • edited

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

xingwu1 commented Jan 27, 2017

tjprescott commented Jan 27, 2017

annatisch commented Jan 27, 2017 • edited

tjprescott commented Jan 27, 2017

annatisch commented Jan 27, 2017

xingwu1 commented Jan 27, 2017

annatisch commented Feb 2, 2017

xingwu1 commented Feb 6, 2017

tjprescott left a comment

Choose a reason for hiding this comment

xingwu1 commented Feb 6, 2017

annatisch commented Feb 7, 2017

troydai commented Feb 7, 2017

Choose a reason for hiding this comment

troydai left a comment •

edited

annatisch commented Jan 26, 2017 •

edited

tjprescott Jan 26, 2017 •

edited

annatisch commented Jan 27, 2017 •

edited