[text analytics] opinion mining support by iscai-msft · Pull Request #12542 · Azure/azure-sdk-for-python

iscai-msft · 2020-07-14T21:32:04Z

Will be updating the samples and readme in a subsequent PR

sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_models.py

kristapratico · 2020-07-15T00:28:00Z

sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_text_analytics_client.py

        docs = _validate_batch_input(documents, "language", language)
        model_version = kwargs.pop("model_version", None)
        show_stats = kwargs.pop("show_stats", False)
+        show_aspects = kwargs.pop("show_aspects", None)


I think this should default to False

I went with None to get the service default, @johanste is the policy to default to None in this case, or False to keep it static

If we need to distinguish between "the application explicitly passed in this value" vs. "the application didn't provide a value, so we'll pick an appropriate default for them" for positional arguments, then we use a sentinel value as the default value. This is often None when None is not a valid value.

Since we are dealing with kwargs here, it would be easier/clearer to check if 'show_aspects' in kwargs: - no need for sentinel values since kwargs inherently let's you determine if the user passed the parameter or not.

@johanste I do need to still pop kwargs in this case since the name of the parameter has changed (we have it as show_aspects, the service has it as opinion_mining). I think I'll stick with the sentinel value of None, holler if this is wrong

sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_text_analytics_client.py

sdk/textanalytics/azure-ai-textanalytics/tests/test_analyze_sentiment.py

sdk/textanalytics/azure-ai-textanalytics/tests/test_analyze_sentiment_async.py

…ervice may change figures

iscai-msft · 2020-07-15T15:53:38Z

/azp run python - textanalytics - ci

johanste · 2020-07-21T16:17:22Z

sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_text_analytics_client.py

        :type documents:
            list[str] or list[~azure.ai.textanalytics.TextDocumentInput] or
            list[dict[str, str]]
+        :keyword bool show_aspects: Whether to conduct more granular analysis around the aspects of


We should add a .. versionadded (or possibly a .. note) directive that indicates in which API version(s) this parameter is available. We should see what the generated docs look like for each since our docs pipelines are customized and I don't think we've used either up until now...

added this code to the PR. While this is not generating great right now (@johanste you mentioned the indentation is off etc), @scbedd is looking into better rendering (thanks Scott!!!!)

…t docstrings

…into opinion_mining * 'master' of https://github.com/Azure/azure-sdk-for-python: (69 commits) [formrecognizer] renames from consistency check (Azure#12752) [text analytics] add back PII endpoint (Azure#12673) [text analytics] generate multiapi client, made v3.1-preview.1 default version (Azure#12720) Prevent AttributeError during get_certificate_version (Azure#12747) re-apply updates after resetting master (Azure#12771) Sync eng/common directory with azure-sdk-tools repository (Azure#12745) Enable bandit for servicebus (Azure#12763) Skip pypy3 for service bus CI pipeline (Azure#12732) Update CODEOWNERS (Azure#12765) updated tox file (Azure#11087) Updating to match Microsoft recommended template (Azure#11045) [text analytics] update version in master (Azure#12749) Explicitly stringify message prints in samples per manual doc pass recommendation. (Azure#12709) Run only analyse dependency step for aggregate report (Azure#12728) VSCodeCredential supports Python 2.7 on Windows (Azure#12731) Enable bandit (Azure#12722) Move InteractiveCredential to a new module (Azure#12706) Add foundations for eng/common restructure (Azure#12725) Sync eng/common directory with azure-sdk-tools repository (Azure#12730) fix flaky test (Azure#12721) ...

kristapratico · 2020-07-30T19:23:12Z

sdk/textanalytics/azure-ai-textanalytics/CHANGELOG.md

 pass in `v3.0` to the kwarg `api_version` when creating your TextAnalyticsClient
 - We have added an API `recognize_pii_entities` which returns entities containing personal information for a batch of documents. Only available for API version v3.1-preview.1 and up.
+- We now have added support for opinion mining. To use this feature, you need to make sure you are using the service's
+v3.1-preview.1 API. To get this support pass `show_opinion_mining` as True when calling the `analyze_sentiment` endpoint


are you gloating

no I'm smiling - like the emoji is

kristapratico · 2020-07-30T19:30:18Z

...tanalytics/azure-ai-textanalytics/azure/ai/textanalytics/aio/_text_analytics_client_async.py

            is not specified, the API will default to the latest, non-preview version.
        :keyword bool show_stats: If set to true, response will contain document level statistics.
+        .. versionadded:: v3.1-preview.1
+            The *show_opinion_mining* parameter.


just curious if this is correct placement or if it should be under the keyword for show_opinion_mining?

If i put it under the definition for show_opinion_mining, it's splitting up the keyword definition now in a very weird way. Speaking of.. @scbedd do you have any updates for this? no rush if not

what if we move show_opinion_mining as the last keyword in docstring? does that help at all?

maririos · 2020-07-30T19:40:10Z

sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_models.py

        and 1 for the sentence for all labels.
    :vartype confidence_scores:
        ~azure.ai.textanalytics.SentimentConfidenceScores
+    :ivar mined_opinions: The list of opinions mined from this sentence.


would it be good to add a ask.ms link to service documentation here about Opinion Mining?

maririos · 2020-07-30T19:47:26Z

sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_text_analytics_client.py

        :type documents:
            list[str] or list[~azure.ai.textanalytics.TextDocumentInput] or
            list[dict[str, str]]
+        :keyword bool show_opinion_mining: Whether to mine the opinions of a sentence and conduct more


could we link here service docs too?

maririos · 2020-07-30T19:48:21Z

sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_text_analytics_client.py

+            granular analysis around the aspects of a product or service (also known as
+            aspect-based sentiment analysis). If set to true, the returned
+            :class:`~azure.ai.textanalytics.SentenceSentiment` objects
+            will have property `mined_opinions` containing the result of this analysis. Only available for


Not sure if we should say this here, as we will have to update it once service GA 3.1
We could just use the link to service docs and let them manage the versions of where the functionality is available

Agreed that it is an extra step of maintenance, but I think we should still have this documentation is here, so it's easier for users to see off the bat that this is only available in certain versions.

Will follow your guidance and open an issue to track this for the future, thanks!

issue here: #12832

maririos · 2020-07-30T19:49:19Z

sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_text_analytics_client.py

+        except TypeError as error:
+            if "opinion_mining" in str(error):
+                raise NotImplementedError(
+                    "'show_opinion_mining' is only available for API version v3.1-preview.1 and up"


early, but we should create an issue to update this value once service GA`s 3.1

issue here: #12832

maririos · 2020-07-30T19:50:53Z

sdk/textanalytics/azure-ai-textanalytics/tests/test_analyze_sentiment.py

+                self.assertIsNotNone(aspect.confidence_scores.positive)
+                self.assertEqual(0.0, aspect.confidence_scores.neutral)
+                self.assertIsNotNone(aspect.confidence_scores.negative)


we could add an extra check where all the confidence scores need to add up to 1. That way we know for sure that not all of them are 0

For more context, in .NET I was doing the exact checks and I missed a bug in the deserialization, so all scores always ended up in 0.0.
The adds up to check has caught those failures

that's fair. i've created a separate issue to add this, since there are more instances in our test suite where i'd like this added: #12833

maririos · 2020-07-30T19:55:27Z

sdk/textanalytics/azure-ai-textanalytics/tests/test_analyze_sentiment.py

+
+    @GlobalTextAnalyticsAccountPreparer()
+    @TextAnalyticsClientPreparer()
+    def test_aspect_based_sentiment_analysis(self, client):


Another test we could add is to turn on opinion mining with a sentence that gives us no results.
For example for the sentence: "today is a hot day"

great suggestion, adding that to this PR. thanks!

maririos

Minor doc and test comments, otherwise LGTM

…v3.0

…into ta_opinion_mining_sample * 'master' of https://github.com/Azure/azure-sdk-for-python: (47 commits) [text analytics] opinion mining support (Azure#12542) [key vault] Regenerate keys (Azure#12101) [Tables] Azure Data Tables SDK, sync and async code (Azure#12766) fix doc, sample, docstring (Azure#12784) clean up links for PII samples (Azure#12826) [formrecognizer] renames from consistency check (Azure#12752) [text analytics] add back PII endpoint (Azure#12673) [text analytics] generate multiapi client, made v3.1-preview.1 default version (Azure#12720) Prevent AttributeError during get_certificate_version (Azure#12747) re-apply updates after resetting master (Azure#12771) Sync eng/common directory with azure-sdk-tools repository (Azure#12745) Enable bandit for servicebus (Azure#12763) Skip pypy3 for service bus CI pipeline (Azure#12732) Update CODEOWNERS (Azure#12765) updated tox file (Azure#11087) Updating to match Microsoft recommended template (Azure#11045) [text analytics] update version in master (Azure#12749) Explicitly stringify message prints in samples per manual doc pass recommendation. (Azure#12709) Run only analyse dependency step for aggregate report (Azure#12728) VSCodeCredential supports Python 2.7 on Windows (Azure#12731) ...

…into regenerate_certs * 'master' of https://github.com/Azure/azure-sdk-for-python: [key vault] Regenerate secrets (Azure#12103) [text analytics] opinion mining support (Azure#12542)

iscai-msft added 6 commits July 14, 2020 11:08

have ta client hold onto its api version in a private property

7755968

implement design for opinion mining

43e72dc

add tests

a3b4510

improve docstrings

32ddecc

made ApiVersion enum all uppercase

ea9fca3

update changelog

590dd11

iscai-msft requested review from annelo-msft, deyaaeldeen, johanste, kinelski, kristapratico, maririos, mssfang, samvaity and willmtemple July 14, 2020 21:32

ghost added the Cognitive - Text Analytics label Jul 14, 2020

kristapratico reviewed Jul 15, 2020

View reviewed changes

sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_models.py Outdated Show resolved Hide resolved

kristapratico reviewed Jul 15, 2020

View reviewed changes

sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_text_analytics_client.py Outdated Show resolved Hide resolved

kristapratico reviewed Jul 15, 2020

View reviewed changes

sdk/textanalytics/azure-ai-textanalytics/tests/test_analyze_sentiment.py Outdated Show resolved Hide resolved

kristapratico reviewed Jul 15, 2020

View reviewed changes

sdk/textanalytics/azure-ai-textanalytics/tests/test_analyze_sentiment_async.py Show resolved Hide resolved

iscai-msft added 3 commits July 15, 2020 10:41

check if positive and negative confidence scores are not None since s…

399571a

…ervice may change figures

improve code flow in sentiment endpoinnt

1ef46f9

pylint

858e70d

iscai-msft added the Client This issue points to a problem in the data-plane of the library. label Jul 15, 2020

iscai-msft self-assigned this Jul 15, 2020

whitespace to update PR

e83b145

iscai-msft closed this Jul 15, 2020

iscai-msft reopened this Jul 15, 2020

johanste reviewed Jul 21, 2020

View reviewed changes

separate out json pointer parsing and add unittest

77c7fa8

iscai-msft requested review from johanste and kristapratico July 21, 2020 19:50

iscai-msft added 4 commits July 21, 2020 15:57

add that show_aspects is only available in v3.1-preview.1 in sentimen…

87b4a80

…t docstrings

switch to kwarg mine_opinions and MinedOpinion body

83426aa

change kwarg from mine_opinions to show_opinion_mining

273fb94

kristapratico reviewed Jul 30, 2020

View reviewed changes

kristapratico previously approved these changes Jul 30, 2020

View reviewed changes

maririos reviewed Jul 30, 2020

View reviewed changes

maririos previously approved these changes Jul 30, 2020

View reviewed changes

iscai-msft added 3 commits July 30, 2020 17:33

change test names from aspect_based_sentiment_analysis to opinion_mining

4180ad0

add test for opinion mining with no mined opinions

5b804d2

have no mined opinions return as [] for v3.1-preview.1, and None for …

f03b477

…v3.0

iscai-msft dismissed stale reviews from maririos and kristapratico via f03b477 July 30, 2020 22:05

iscai-msft requested a review from kristapratico July 30, 2020 22:06

maririos approved these changes Jul 30, 2020

View reviewed changes

kristapratico approved these changes Jul 30, 2020

View reviewed changes

iscai-msft merged commit 2c4182c into Azure:master Jul 31, 2020

iscai-msft deleted the opinion_mining branch July 31, 2020 14:56

Conversation

iscai-msft commented Jul 14, 2020

Uh oh!

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!

iscai-msft Jul 21, 2020 • 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!

Uh oh!

Uh oh!

Uh oh!

iscai-msft commented Jul 15, 2020

Uh oh!

johanste Jul 21, 2020 • 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!

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!

maririos Jul 30, 2020 • 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!

maririos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iscai-msft Jul 21, 2020 •

edited

Loading

johanste Jul 21, 2020 •

edited

Loading

maririos Jul 30, 2020 •

edited

Loading