Skip to content

[TA] Adding support for PII endpoint #13687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 13 commits into from
Aug 20, 2020
Merged

[TA] Adding support for PII endpoint #13687

merged 13 commits into from
Aug 20, 2020

Conversation

mssfang
Copy link
Contributor

@mssfang mssfang commented Jul 31, 2020 •
edited
Loading

@mssfang mssfang added Client This issue points to a problem in the data-plane of the library. Cognitive - Text Analytics labels Jul 31, 2020
@mssfang mssfang added this to the [2020] August milestone Jul 31, 2020
@mssfang mssfang self-assigned this Jul 31, 2020
@mssfang mssfang requested a review from samvaity July 31, 2020 23:50
@mssfang mssfang marked this pull request as ready for review August 3, 2020 16:35
@mssfang mssfang requested a review from sima-zhu as a code owner August 3, 2020 16:35
iscai-msft
iscai-msft reviewed Aug 3, 2020
View reviewed changes
@maririos maririos mentioned this pull request Aug 7, 2020
Closed
samvaity
samvaity reviewed Aug 12, 2020
View reviewed changes
sdk/textanalytics/azure-ai-textanalytics/README.md Outdated
Comment on lines 280 to 281
For samples on using the production recommended option `RecognizePiiEntitiesBatch` see [here][recognize_pii_entities_sample].
Please refer to the service documentation for [supported PII entity types][pii_entity_recognition].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we do this section for all the readme snippets if not consider moving this to Next Steps?

Copy link
Contributor Author

@mssfang mssfang Aug 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not getting what you mean here. It should already for all the README snippets. It follows the pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested if this could be moved to Next Steps section.

@mssfang mssfang requested a review from samvaity August 14, 2020 06:30
@maririos maririos self-requested a review August 17, 2020 22:55
@maririos
Copy link
Member

Looking at Azure/azure-rest-api-specs#10440 should we generate the PII code using 3.2-preview.1?

@mssfang
Copy link
Contributor Author

mssfang commented Aug 19, 2020

Looking at Azure/azure-rest-api-specs#10440 should we generate the PII code using 3.2-preview.1?

if time is applicable, we should use 3.2-preview.1 instead of 3.1-preview.1. But I will do it in a new PR.

@mssfang mssfang requested a review from iscai-msft August 19, 2020 14:56
mssfang
mssfang commented Aug 19, 2020
View reviewed changes
sdk/textanalytics/azure-ai-textanalytics/src/samples/README.md Outdated
@@ -82,6 +88,9 @@ This project welcomes contributions and suggestions. Find [more contributing][SD
[async_sample_entities]: https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/textanalytics/azure-ai-textanalytics/src/samples/java/com/azure/ai/textanalytics/RecognizeEntitiesAsync.java
[async_sample_entities_batch]: https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/textanalytics/azure-ai-textanalytics/src/samples/java/com/azure/ai/textanalytics/batch/RecognizeEntitiesBatchDocumentsAsync.java
[async_sample_entities_batch_convenience]: https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/textanalytics/azure-ai-textanalytics/src/samples/java/com/azure/ai/textanalytics/batch/RecognizeEntitiesBatchStringDocumentsAsync.java
[async_sample_pii_entities]: https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/textanalytics/azure-ai-textanalytics/src/samples/java/com/azure/ai/textanalytics/RecognizePiiEntitiesAsync.java
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't pass ci because of broken link, so will add them back after the sample merged.

https://dev.azure.com/azure-sdk/public/_build/results?buildId=501818&view=logs&j=36df4387-3551-57fa-098c-b43a23930261&t=3851d55a-4253-57f5-016e-927e8de9d21b

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#14274

samvaity
samvaity reviewed Aug 20, 2020
View reviewed changes
...ics/azure-ai-textanalytics/src/main/java/com/azure/ai/textanalytics/TextAnalyticsClient.java
@ServiceMethod(returns = ReturnType.SINGLE)
public RecognizePiiEntitiesResultCollection recognizePiiEntitiesBatch(
Iterable<String> documents, String language, TextAnalyticsRequestOptions options) {
return client.recognizePiiEntitiesBatch(documents, language, options).block();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can call the method on L403.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is better to link to call async version of it rather than depends on other sync overload.

...ics/azure-ai-textanalytics/src/main/java/com/azure/ai/textanalytics/TextAnalyticsClient.java
@ServiceMethod(returns = ReturnType.SINGLE)
public PiiEntityCollection recognizePiiEntities(String document, String language) {
Objects.requireNonNull(document, "'document' cannot be null.");
return client.recognizePiiEntities(document, language).block();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this call the method on L373

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same reason as in #13687 (comment)

samvaity
samvaity approved these changes Aug 20, 2020
View reviewed changes
Copy link
Member

@samvaity samvaity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from a few comments, changes lgtm

@mssfang
Copy link
Contributor Author

mssfang commented Aug 20, 2020

@samvaity
#13687 (comment)

if (entitiesResult.isError()) {
  throw logger.logExceptionAsError(toTextAnalyticsException(entitiesResult.getError()));
}

atomic API internally call the batch APIs, which is the single entities document result. If the result is an error (TextAnalyticsError), then we return it as TextAnalyticsException.

@mssfang mssfang merged commit 0a58771 into Azure:master Aug 20, 2020
@mssfang mssfang deleted the TA-PII branch August 20, 2020 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maririos maririos maririos approved these changes

@samvaity samvaity samvaity approved these changes

@sima-zhu sima-zhu Awaiting requested review from sima-zhu

@willmtemple willmtemple Awaiting requested review from willmtemple

@deyaaeldeen deyaaeldeen Awaiting requested review from deyaaeldeen

@kristapratico kristapratico Awaiting requested review from kristapratico

@iscai-msft iscai-msft Awaiting requested review from iscai-msft

Assignees

@mssfang mssfang

Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Text Analytics
Projects
None yet
Milestone
[2020] September
Development

Successfully merging this pull request may close these issues.

[TA] Add PII endpoint feature
4 participants
@mssfang @maririos @samvaity @iscai-msft