azure cognitive service CLI initial version #3060

kostenray · 2017-04-28T05:00:32Z

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

Each command and parameter has a meaningful description.
Each new command has a test.

msftclas · 2017-04-28T05:00:38Z

@kostenray,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

vishrutshah

It'd be great if you can post help on each of the commands that helps reviewing the commands

vishrutshah · 2017-04-28T05:08:44Z

src/azure-cli-testsdk/azure/cli/testsdk/base.py

    def __init__(self, method_name):
        super(IntegrationTestBase, self).__init__(method_name)
-        self.diagnose = os.environ.get(ENV_TEST_DIAGNOSE, None) == 'True'
+        self.diagnose = True #os.environ.get(ENV_TEST_DIAGNOSE, None) == 'True'


Why are you changing azure-cli-testsdk?

Please remove this.

it's checked in accidentally. removed

As of now, it is still here.

vishrutshah · 2017-04-28T05:09:57Z

...s/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/_client_factory.py

+def cognitiveservices_client_factory(_):
+    from azure.cli.core.commands.client_factory import get_mgmt_service_client
+    from azure.mgmt.cognitiveservices import CognitiveServicesManagementClient
+    return get_mgmt_service_client(CognitiveServicesManagementClient, location='notused').cognitive_services_accounts


could you please elaborate what is location='notused' ?

there is an extra parameter need passing a value when initializing the sdk management client. but it's not used by every class and operation. so for some initialization, i need pass a dummy value.

vishrutshah · 2017-04-28T05:10:01Z

...s/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/_client_factory.py

+def cognitiveservices_account_client_factory(_):
+    from azure.cli.core.commands.client_factory import get_mgmt_service_client
+    from azure.mgmt.cognitiveservices import CognitiveServicesManagementClient
+    return get_mgmt_service_client(CognitiveServicesManagementClient, location='notused').accounts


could you please elaborate what is location='notused' ?

see above comments

vishrutshah · 2017-04-28T05:13:05Z

...s/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/_client_factory.py

+    from azure.mgmt.cognitiveservices import CognitiveServicesManagementClient
+    return get_mgmt_service_client(CognitiveServicesManagementClient, location='notused').cognitive_services_accounts
+
+def cognitiveservices_account_client_factory(_):


Refactor by adding get_ cognitiveservices_management_client and then use them in cognitiveservices_account_client_factory & other operations

Reference: https://github.com/Azure/azure-cli/blob/master/src/command_modules/azure-cli-monitor/azure/cli/command_modules/monitor/_client_factory.py#L8

vishrutshah · 2017-04-28T05:13:37Z

...d_modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/_params.py

+register_cli_argument('cognitiveservices', 'tags', tags_type)
+register_cli_argument('cognitiveservices', 'key_name', required=True, help='Key name to generate', choices=['key1', 'key2'])
+register_cli_argument('cognitiveservices account create', 'yes', action='store_true', help='Do not prompt for terms confirmation')
+


please remove unnecessary lines

vishrutshah · 2017-04-28T05:18:49Z

...nd_modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/custom.py

+
+def create(client, resource_group_name, account_name, sku_name, kind, location, tags=None, yes=None, **kwargs):
+
+    terms = 'Notice\nMicrosoft may use data you send to the Cognitive Services to improve Microsoft products and services. For example we may use content that you provide to the Cognitive Services to improve our underlying algorithms and models over time. Where you send personal data to the Cognitive Services, you are responsible for obtaining sufficient consent from the data subjects. The General Privacy and Security Terms in the Online Services Terms (https://www.microsoft.com/en-us/Licensing/product-licensing/products.aspx) do not apply to the Cognitive Services. You must comply with use and display requirements for the Bing Search APIs. \n\nPlease refer to the Microsoft Cognitive Services section in the Online Services Terms for details.'


@troydai / @tjprescott Do we have any other recommendation instead of this mechanism?

We have confirmation mechanism, but this is the first case for a warning. If it is for user consumption the message should be print to stdout instead of logger. The logger may be turned off.

won't fix as it's fine

vishrutshah · 2017-04-28T05:19:06Z

...nd_modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/custom.py

+def update(client, resource_group_name, account_name, sku_name=None, tags=None, **kwargs):
+    sku =  Sku(sku_name)
+    return client.update(resource_group_name, account_name, sku, tags)
+


unnecessary extra lines

vishrutshah · 2017-04-28T05:19:25Z

...nd_modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/custom.py

+    params = CognitiveServicesAccountCreateParameters(sku, kind, location, properties, tags)
+    return client.create(resource_group_name, account_name, params)
+
+def update(client, resource_group_name, account_name, sku_name=None, tags=None, **kwargs):


how are you using **kwargs ?

vishrutshah · 2017-04-28T05:20:00Z

...nd_modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/custom.py

+        if(not option):
+             raise CLIError('Operation cancelled.')
+    sku =  Sku(sku_name)
+    properties= {}


If properties are going to be empty dictionary, why are they not options in SDK?

vishrutshah · 2017-04-28T05:21:05Z

src/command_modules/azure-cli-cognitiveservices/setup.py

+]
+
+DEPENDENCIES = [
+]


https://pypi.python.org/pypi/azure-mgmt-cognitiveservices ?

troydai

Please fix the code styles.

troydai · 2017-04-28T15:55:21Z

src/azure-cli-testsdk/azure/cli/testsdk/base.py

    def __init__(self, method_name):
        super(IntegrationTestBase, self).__init__(method_name)
-        self.diagnose = os.environ.get(ENV_TEST_DIAGNOSE, None) == 'True'
+        self.diagnose = True #os.environ.get(ENV_TEST_DIAGNOSE, None) == 'True'


Please remove this.

troydai · 2017-04-28T15:56:32Z

..._modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/__init__.py

+
+def load_commands():
+    import azure.cli.command_modules.cognitiveservices.commands
+


Please run check_style --module cognitiveservices to fix all the code style issues. This file will fail the pylint because we require 2 blank lines between first level entities.

troydai · 2017-04-28T15:57:15Z

...and_modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/_help.py

+#helps['cognitiveservices checkskuavailability'] = """
+#    type: command
+#    short-summary: check the available skus
+#"""


Please do not check in commented code.

Do you mean the command is obsoleted or the help doc is obsoleted? Either way, this should be removed from the source code.

troydai · 2017-04-28T15:58:22Z

...d_modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/_params.py

+# Licensed under the MIT License. See License.txt in the project root for license information.
+# --------------------------------------------------------------------------------------------
+
+# pylint: disable=line-too-long


Please do not disable pylint rules in a file scope. Make the follow the style instead of bending the rules. There are existing pylint disable statement in the legacy code base, we're in the process of retiring them.

troydai · 2017-04-28T15:58:50Z

..._modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/commands.py

+# Licensed under the MIT License. See License.txt in the project root for license information.
+# --------------------------------------------------------------------------------------------
+
+# pylint: disable=line-too-long


Same as above.

troydai · 2017-04-28T16:00:32Z

...nd_modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/custom.py

+
+def create(client, resource_group_name, account_name, sku_name, kind, location, tags=None, yes=None, **kwargs):
+
+    terms = 'Notice\nMicrosoft may use data you send to the Cognitive Services to improve Microsoft products and services. For example we may use content that you provide to the Cognitive Services to improve our underlying algorithms and models over time. Where you send personal data to the Cognitive Services, you are responsible for obtaining sufficient consent from the data subjects. The General Privacy and Security Terms in the Online Services Terms (https://www.microsoft.com/en-us/Licensing/product-licensing/products.aspx) do not apply to the Cognitive Services. You must comply with use and display requirements for the Bing Search APIs. \n\nPlease refer to the Microsoft Cognitive Services section in the Online Services Terms for details.'


We have confirmation mechanism, but this is the first case for a warning. If it is for user consumption the message should be print to stdout instead of logger. The logger may be turned off.

troydai · 2017-04-28T16:01:38Z

src/command_modules/azure-cli-cognitiveservices/tests/test_cognitiveservices_command.py

+
+#pylint: disable=method-hidden
+#pylint: disable=line-too-long
+#pylint: disable=bad-continuation


Please remove pylint disable statements.

troydai · 2017-04-28T16:02:06Z

src/command_modules/azure-cli-cognitiveservices/tests/test_cognitiveservices_command.py

+class CognitiveServicesTests(ScenarioTest):
+    @ResourceGroupPreparer()
+
+    def test_cognitiveservices(self, resource_group):


Please use a better and more elaborate test name. There will be more test cases eventually.

derekbekoe · 2017-04-28T23:53:34Z

src/command_modules/azure-cli-cognitiveservices/setup.py

+
+from codecs import open
+from setuptools import setup
+


The setup.py should have the following:

try: from azure_bdist_wheel import cmdclass except ImportError: from distutils import log as logger logger.warn("Wheel is not available, disabling bdist_wheel hook") cmdclass = {}

Also, it should have cmdclass=cmdclass in setup(...). See other modules for examples.

cc: @johanste

derekbekoe · 2017-04-28T23:54:20Z

src/command_modules/azure-cli-cognitiveservices/HISTORY.rst

+
+Unreleased
+--------------------------
+* inital cognitive services CLI


This section should go below the 'Release History' section.

derekbekoe · 2017-04-28T23:55:18Z

...and_modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/_help.py

+    short-summary: Create a cognitive services account.
+    examples:
+        - name: create a S0 face Api cognitive services accounts in West Europe without confirmation required
+          text: az cognitiveservices create -n myresource -g myResrouceGroup --Kind Face --Sku S0 -l WestEurope --yes


Typo myResrouceGroup.
Also, Kind -> kind.

derekbekoe · 2017-04-28T23:56:57Z

src/command_modules/azure-cli-cognitiveservices/setup.py

+
+setup(
+    name='azure-cli-cognitiveservices',
+    version='1.0.0',


Version number should be 0.x.x e.g. 0.1.0

fixed. is this because it's not released version yet?

derekbekoe · 2017-04-28T23:57:18Z

src/command_modules/azure-cli-cognitiveservices/setup.py

+        'azure',
+        'azure.cli',
+        'azure.cli.command_modules',
+    ],


This has changed recently.
See other modules for examples.

cc: @johanste

i checked the other modules, and they seems to be the same

troydai

Please run check_style --module cognitiveservices to clear the code style issues.

troydai · 2017-05-01T20:33:04Z

src/command_modules/azure-cli-cognitiveservices/tests/test_cognitiveservices_command.py

    @ResourceGroupPreparer()

-    def test_cognitiveservices(self, resource_group):
+    def test_CRUDOperations_cognitiveservices(self, resource_group):


The format adopted by other command module is:

test_<module>_<scenario>

troydai · 2017-05-01T20:33:51Z

src/azure-cli-testsdk/azure/cli/testsdk/base.py

    def __init__(self, method_name):
        super(IntegrationTestBase, self).__init__(method_name)
-        self.diagnose = os.environ.get(ENV_TEST_DIAGNOSE, None) == 'True'
+        self.diagnose = True #os.environ.get(ENV_TEST_DIAGNOSE, None) == 'True'


As of now, it is still here.

troydai · 2017-05-01T20:34:52Z

...and_modules/azure-cli-cognitiveservices/azure/cli/command_modules/cognitiveservices/_help.py

+#helps['cognitiveservices checkskuavailability'] = """
+#    type: command
+#    short-summary: check the available skus
+#"""


Do you mean the command is obsoleted or the help doc is obsoleted? Either way, this should be removed from the source code.

troydai · 2017-05-01T20:36:23Z

src/command_modules/azure-cli-cognitiveservices/setup.py

@@ -0,0 +1,56 @@
+#!/usr/bin/env python


We do not need shebang on setup file.

2. add the depedencies in setup,

codecov-io · 2017-05-02T18:33:28Z

Codecov Report

Merging #3060 into master will increase coverage by 7.42%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #3060      +/-   ##
==========================================
+ Coverage   63.15%   70.57%   +7.42%     
==========================================
  Files         484      369     -115     
  Lines       27628    23902    -3726     
  Branches     4281     3657     -624     
==========================================
- Hits        17449    16870     -579     
+ Misses       9027     5956    -3071     
+ Partials     1152     1076      -76

Impacted Files	Coverage Δ
.../cli/command_modules/cognitiveservices/commands.py	`100% <100%> (ø)`
.../azure-cli-cognitiveservices/azure/cli/__init__.py	`100% <100%> (ø)`
...ules/azure-cli-cognitiveservices/azure/__init__.py	`100% <100%> (ø)`
...mmand_modules/cognitiveservices/_client_factory.py	`100% <100%> (ø)`
.../cli/command_modules/cognitiveservices/__init__.py	`100% <100%> (ø)`
...e/cli/command_modules/cognitiveservices/_params.py	`100% <100%> (ø)`
...ure/cli/command_modules/cognitiveservices/_help.py	`100% <100%> (ø)`
...tiveservices/azure/cli/command_modules/__init__.py	`100% <100%> (ø)`
...re/cli/command_modules/cognitiveservices/custom.py	`72% <72%> (ø)`
.../cli/command_modules/appservice/_client_factory.py	`35.71% <0%> (-64.29%)`	⬇️
... and 238 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 c97edcf...9065ccc. Read the comment docs.

troydai · 2017-05-02T18:41:33Z

I'm taking another look

derekbekoe

A couple of changes are required in setup.py but apart from that, it looks good!

derekbekoe · 2017-05-02T18:45:11Z

src/command_modules/azure-cli-cognitiveservices/setup.py

+
+setup(
+    name='azure-cli-cognitiveservices',
+    version='0.1.0',


The version no. should be defined like in the other modules.
So after line 14, VERSION = '0.1.0+dev'.
Then here, version=VERSION,.
See the setup.py of other modules for an example.

fixedLeave a comment

derekbekoe · 2017-05-02T18:45:32Z

src/command_modules/azure-cli-cognitiveservices/setup.py

+    name='azure-cli-cognitiveservices',
+    version='0.1.0',
+    description='Microsoft Azure Command-Line Tools Cognitive Services Command Module',
+    long_description=README,


This does not include the HISTORY.rst. See the other setup.py files for examples.

Dismissing as he's oof.

Ray Xi and others added 7 commits April 24, 2017 12:47

initial update

1e026ee

update

b5b14bf

add the CLI for 2017-04-18 swagger file

1a77bb5

cognitive service cli - inital version'

96f6eb0

rollback debug code change

7806815

style refactoring update

a0763a6

update

fc47810

msftclas added the cla-not-required label Apr 28, 2017

vishrutshah previously requested changes Apr 28, 2017

View reviewed changes

troydai suggested changes Apr 28, 2017

View reviewed changes

derekbekoe suggested changes Apr 28, 2017

View reviewed changes

kostenray added 2 commits April 28, 2017 20:34

change the term text to yellow highlighted

d26c098

code comment fix - update #1

1b9e313

troydai suggested changes May 1, 2017

View reviewed changes

derekbekoe added this to the Build Milestone milestone May 1, 2017

kostenray added 4 commits May 1, 2017 16:14

fix code comments - update Azure#2

b6c6643

1.fix flake 8 styling error

25adc16

2. add the depedencies in setup,

fix an change in our service legal terms.

17db72d

fix a help.py styling issue

25dd3ea

troydai approved these changes May 2, 2017

View reviewed changes

derekbekoe suggested changes May 2, 2017

View reviewed changes

kostenray added 2 commits May 2, 2017 13:48

update the setup.py

410e490

update description of setup.py

9065ccc

derekbekoe approved these changes May 2, 2017

View reviewed changes

derekbekoe merged commit 21e1f9d into Azure:master May 2, 2017

haroldrandom added the cla-not-required label Oct 25, 2019


		def create(client, resource_group_name, account_name, sku_name, kind, location, tags=None, yes=None, **kwargs):

		terms = 'Notice\nMicrosoft may use data you send to the Cognitive Services to improve Microsoft products and services. For example we may use content that you provide to the Cognitive Services to improve our underlying algorithms and models over time. Where you send personal data to the Cognitive Services, you are responsible for obtaining sufficient consent from the data subjects. The General Privacy and Security Terms in the Online Services Terms (https://www.microsoft.com/en-us/Licensing/product-licensing/products.aspx) do not apply to the Cognitive Services. You must comply with use and display requirements for the Bing Search APIs. \n\nPlease refer to the Microsoft Cognitive Services section in the Online Services Terms for details.'


		def load_commands():
		import azure.cli.command_modules.cognitiveservices.commands

azure cognitive service CLI initial version #3060

azure cognitive service CLI initial version #3060

Uh oh!

Conversation

kostenray commented Apr 28, 2017

General Guidelines

Command Guidelines

Uh oh!

msftclas commented Apr 28, 2017

Uh oh!

vishrutshah 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

troydai 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!

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!