[TA] Added RecognizeCustomEntities Functionality by ZulaMostafa · Pull Request #24245 · Azure/azure-sdk-for-net

ZulaMostafa · 2021-09-27T14:35:19Z

This PR contains the implementation of the Recognize Custom Entities API interface as explained in #24054

sdk/textanalytics/Azure.AI.TextAnalytics/CHANGELOG.md

maririos

So far it looks good. Mostly nits. Next review I will look at the tests

Because Ahmed's PR was merged, need to resolve the conflicts.

sdk/textanalytics/Azure.AI.TextAnalytics/CHANGELOG.md

sdk/textanalytics/Azure.AI.TextAnalytics/samples/Sample9_RecognizeCustomEntities.md

sdk/textanalytics/Azure.AI.TextAnalytics/src/Internal/CustomEntitiesTaskParameters.cs

sdk/textanalytics/Azure.AI.TextAnalytics/src/JobManifestTasks.cs

sdk/textanalytics/Azure.AI.TextAnalytics/src/RecognizeCustomEntitiesAction.cs

sdk/textanalytics/Azure.AI.TextAnalytics/src/TextAnalyticsModelFactory.cs

sdk/textanalytics/Azure.AI.TextAnalytics/CHANGELOG.md

sdk/textanalytics/Azure.AI.TextAnalytics/samples/Sample9_RecognizeCustomEntities.md

sdk/textanalytics/Azure.AI.TextAnalytics/src/RecognizeCustomEntitiesResultCollection.cs

deyaaeldeen

The implementation looks really good. I left a few comments, with one on re-using RecognizeEntitiesResult.

sdk/textanalytics/Azure.AI.TextAnalytics/src/autorest.md

ZulaMostafa · 2021-10-03T11:35:19Z

@maririos @mssfang @deyaaeldeen Thank you all for your great review! I've made all the changes, I've also generated the code based on the new swagger as @mssfang told me.
Since @AhmedLeithy rebased his other PR I will wait for his to get merged, to not rebase 2 times.

check-enforcer · 2021-10-03T11:45:30Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them. In order to bootstrap pipelines for a new service, please perform following steps:

For data-plane/track 2 SDKs Issue the following command as a pull request comment:

/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run net - [service] - ci

For track 1 management-plane SDKs

Please open a separate PR and to your service SDK path in this file. Once that PR has been merged, you can re-run the pipeline to trigger the verification.

sdk/textanalytics/Azure.AI.TextAnalytics/samples/Sample9_RecognizeCustomEntities.md

sdk/textanalytics/Azure.AI.TextAnalytics/CHANGELOG.md

kinelski

Partial review (haven't looked through tests yet).

sdk/textanalytics/Azure.AI.TextAnalytics/CHANGELOG.md

sdk/textanalytics/Azure.AI.TextAnalytics/src/AnalyzeActionsResult.cs

sdk/textanalytics/Azure.AI.TextAnalytics/src/Internal/CustomEntitiesTaskParameters.cs

sdk/textanalytics/Azure.AI.TextAnalytics/src/TextAnalyticsModelFactory.cs

sdk/textanalytics/Azure.AI.TextAnalytics/src/Transforms.cs

...tics/Azure.AI.TextAnalytics/tests/samples/Sample9_RecognizeCustomEntitiesAsyncConvenience.cs

sdk/textanalytics/Azure.AI.TextAnalytics/tests/Infrastructure/TextAnalyticsTestEnvironment.cs

...tics/Azure.AI.TextAnalytics/tests/samples/Sample9_RecognizeCustomEntitiesAsyncConvenience.cs

sdk/textanalytics/Azure.AI.TextAnalytics/tests/samples/Sample9_RecognizeCustomEntities.cs

sdk/textanalytics/Azure.AI.TextAnalytics/CHANGELOG.md

sdk/textanalytics/Azure.AI.TextAnalytics/samples/Sample9_RecognizeCustomEntities.md

sdk/textanalytics/Azure.AI.TextAnalytics/src/RecognizeCustomEntitiesAction.cs

sdk/textanalytics/Azure.AI.TextAnalytics/src/Transforms.cs

maririos

Almost there. Most comments are nits.

sdk/textanalytics/Azure.AI.TextAnalytics/samples/Sample9_RecognizeCustomEntities.md

sdk/textanalytics/Azure.AI.TextAnalytics/tests/RecognizeCustomEntitiesTests.cs

maririos · 2021-10-18T17:33:53Z

sdk/textanalytics/Azure.AI.TextAnalytics/tests/RecognizeCustomEntitiesTests.cs

+            return ActionResult.DocumentsResults;
+        }
+
+        private void ValidateInDocumentResult(CategorizedEntityCollection entities, List<string> minimumExpectedOutput)


For now, this is good, but could you create an issue for us to look into moving the ValidateInDocumentResult function into a commonplace that both the RecognizeCustomEntitiesTests and the RecognizeEntitiesTests classes can consume?

....TextAnalytics/tests/SessionRecords/AnalyzeOperationTests/AnalyzeOperationWithPHIDomain.json

sdk/textanalytics/Azure.AI.TextAnalytics/tests/TextAnalyticsModelFactoryTests.cs

maririos · 2021-10-18T17:41:46Z

sdk/textanalytics/azure.ai.textanalytics/src/JobManifestTasks.cs

+    /// <summary>
+    /// JobManifestTasks.
+    /// </summary>
+    [CodeGenModel("JobManifestTasks")]


I imagine this was added by mistake?

This is actually so weird, I didn't add it and I have no idea where did it come from!

The difference between it and the original one is the path, azure.ai.textanalytics is capitalized in the original one. I can't find the file locally, It may be a bug

weird. can u just delete the file so we don't merge it?

maririos · 2021-10-18T17:44:48Z

@ZulaMostafa after you make changes to samples that use snippets, don't forget to run eng\scripts\Update-Snippets.ps1 textanalytics so the snippets are updated everywhere and you don't get build breaks like this one:

https://dev.azure.com/azure-sdk/public/_build/results?buildId=1147189&view=logs&j=b70e5e73-bbb6-5567-0939-8415943fadb9&t=2102385d-609d-5572-64d2-932661c7902f

maririos

Besides the weird new file :P changes LGTM!!!

ZulaMostafa · 2021-10-19T20:40:10Z

I think we are good to go now!

maririos · 2021-10-19T20:46:20Z

I think we are good to go now!

Almost. Still need to delete file sdk/textanalytics/azure.ai.textanalytics/src/JobManifestTasks.cs

ZulaMostafa · 2021-10-19T20:50:59Z

I think we are good to go now!

Almost. Still need to delete file sdk/textanalytics/azure.ai.textanalytics/src/JobManifestTasks.cs

My bad, I read your comment from the mail so I thought you was talking about session records. Anyways, I deleted the weirdo file :p

maririos · 2021-10-19T21:14:40Z

/azp run net - textanalytics - ci

azure-pipelines · 2021-10-19T21:14:56Z

Azure Pipelines successfully started running 1 pipeline(s).

* [TextAnalytics] Generated client from 3.2-preview.2 swagger (#23536) * [TA] Added SingleCategoryClassify functionality (#24235) * [TA] Added MultiCategoryClassify Functionality (#24237) * [TA] Added RecognizeCustomEntities Functionality (#24245) * [TA] Expose ActionName and enable multiple actions from same type (#24619) * Rerecorded all tests excluding AAD ones (#24913) * re-record AAD tests (#24919) * [TA] Enable CI for live tests for custom features (#24916) * add comments Co-authored-by: Caio Saldanha <camaiaor@microsoft.com> Co-authored-by: Ahmed Leithy <v-aleithy@microsoft.com> Co-authored-by: Salah Mostafa <zulamostafa@gmail.com> Co-authored-by: Salah Mostafa <v-samostafa@microsoft.com>

ZulaMostafa requested review from annelo-msft and maririos as code owners September 27, 2021 14:35

ghost added the Cognitive - Text Analytics label Sep 27, 2021

ZulaMostafa mentioned this pull request Sep 27, 2021

[TA] Added Recognize Custom Entities Action #24139

Closed

ZulaMostafa commented Sep 27, 2021

View reviewed changes

sdk/textanalytics/Azure.AI.TextAnalytics/CHANGELOG.md Outdated Show resolved Hide resolved

maririos requested review from deyaaeldeen, kinelski, kristapratico and mssfang September 29, 2021 17:55

maririos reviewed Sep 29, 2021

View reviewed changes

deyaaeldeen reviewed Sep 29, 2021

View reviewed changes

sdk/textanalytics/Azure.AI.TextAnalytics/CHANGELOG.md Outdated Show resolved Hide resolved

deyaaeldeen reviewed Sep 29, 2021

View reviewed changes

sdk/textanalytics/Azure.AI.TextAnalytics/samples/Sample9_RecognizeCustomEntities.md Outdated Show resolved Hide resolved

deyaaeldeen reviewed Sep 29, 2021

View reviewed changes

sdk/textanalytics/Azure.AI.TextAnalytics/src/RecognizeCustomEntitiesResultCollection.cs Show resolved Hide resolved

deyaaeldeen reviewed Sep 29, 2021

View reviewed changes

mssfang reviewed Sep 29, 2021

View reviewed changes

sdk/textanalytics/Azure.AI.TextAnalytics/src/autorest.md Outdated Show resolved Hide resolved

ZulaMostafa force-pushed the TA-CustomNER branch from bffce62 to b95eab9 Compare October 6, 2021 14:12

ZulaMostafa force-pushed the TA-CustomNER branch from b95eab9 to c67e98a Compare October 6, 2021 15:43

ZulaMostafa force-pushed the TA-CustomNER branch from c67e98a to 26c7640 Compare October 6, 2021 16:09

ZulaMostafa requested review from KrzysztofCwalina and tg-msft as code owners October 6, 2021 16:09

mssfang reviewed Oct 6, 2021

View reviewed changes

sdk/textanalytics/Azure.AI.TextAnalytics/samples/Sample9_RecognizeCustomEntities.md Outdated Show resolved Hide resolved

[TA] Added RecognizeCustomEntities Functionality (Azure#24245)

dded44f

ZulaMostafa force-pushed the TA-CustomNER branch from 26c7640 to dded44f Compare October 6, 2021 16:50

maririos reviewed Oct 6, 2021

View reviewed changes

sdk/textanalytics/Azure.AI.TextAnalytics/CHANGELOG.md Outdated Show resolved Hide resolved

kinelski reviewed Oct 6, 2021

View reviewed changes

maririos requested a review from jsquire October 7, 2021 21:11

Changes based on review

9993deb

jsquire reviewed Oct 11, 2021

View reviewed changes

Salah Mostafa added 2 commits October 17, 2021 15:34

minor changes based on review

2886506

minor changes

a405ac8

maririos reviewed Oct 18, 2021

View reviewed changes

minor changes

2335135

maririos approved these changes Oct 19, 2021

View reviewed changes

undid non-needed records

75c450b

deleted JobManifestsTasks.cs

1d6666d

maririos merged commit 6b074dd into Azure:feature/textanalytics/custom Oct 19, 2021

maririos pushed a commit to maririos/azure-sdk-for-net that referenced this pull request Oct 26, 2021

[TA] Added RecognizeCustomEntities Functionality (Azure#24245)

a4b0e8a

Conversation

ZulaMostafa commented Sep 27, 2021

Uh oh!

Uh oh!

maririos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deyaaeldeen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZulaMostafa commented Oct 3, 2021

Uh oh!

check-enforcer bot commented Oct 3, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

For data-plane/track 2 SDKs Issue the following command as a pull request comment:

For track 1 management-plane SDKs

Uh oh!

Uh oh!

Uh oh!

kinelski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maririos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maririos Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maririos Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

ZulaMostafa Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

ZulaMostafa Oct 19, 2021