Skip to content
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.

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

[TA] Added SingleCategoryClassify functionality #24235

Conversation

AhmedLeithy
Copy link
Member

@AhmedLeithy AhmedLeithy commented Sep 26, 2021

This PR contains the implementation of the Single Category Classify API interface as explained in #24061. Renames post the meeting with the architecture board have been taken into consideration in this PR.

The two known issues are:

  • "projectName" and "deploymentName" are expected to be "project-name" and "deployment-name" by the PPE. This change can also be temporarily made in CustomSingleClassificationTaskParameters.Serialization.cs

  • the PPE does not currently support old actions, and thus session records for them cannot yet be rerecorded.

No long-term solution can currently be done about the previous two issues, but they can be solved in another PR.

@maririos
Copy link
Member

"projectName" and "deploymentName" are expected to be "project-name" and "deployment-name" by the PPE. This change can also be temporarily made in CustomSingleClassificationTaskParameters.Serialization.cs

You can't modify the **.Serialization classes as every time codegen is run, your changes will be overwritten.
I don't have much information on the specific issue, so a couple of questions (@mssfang , @deyaaeldeen):

  • Has this been reported to the service team?
  • Is there an ETA on when the fix from the service team will happen?

If you want to "trick" the SDK so that it reflects what the current behavior of the service is, you could write a transform and add it to the autorest.md file that will rename the parameter from projectName to project-name. The transforms are applied to the swagger before all the code is generated, so every time you run codegen, it will take into account the transform. If you want to do this, let me know and I can help write the transform fi you want. I have done tons of transforms lately :P

When the service fixes the bug, you can undo the transform.

@deyaaeldeen
Copy link
Member

I agree with what @maririos said. Fortunately, the swagger got updated today and this issue has been fixed so you just need to re-generate with the latest version.

Copy link
Member

@maririos maririos left a comment

Choose a reason for hiding this comment

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

Comments so far from .NET implementation perspective. Didn't look at test or sample implementation.

@deyaaeldeen and @mssfang could you review from the design and terminology perspective?

// View operation results.
await foreach (AnalyzeActionsResult documentsInPage in operation.Value)
{
IReadOnlyCollection<SingleCategoryClassifyActionResult> classificationResultsCollection = documentsInPage.SingleCategoryClassifyResults;
Copy link
Member

Choose a reason for hiding this comment

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

is singleClassificationActionResults? a better name? I thought classificationResultsCollection is the document result collections.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. So you think I should replace classificationResultsCollection with singleClassificationActionResults. Correct?

Copy link
Member

@maririos maririos left a comment

Choose a reason for hiding this comment

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

PR LGTM :) Just need to re-record some failing tests and make sure CI passes

Copy link
Member

@maririos maririos left a comment

Choose a reason for hiding this comment

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

Talked offline where Ahmed reminded me that not all endpoints are deployed so test failing is expected

@maririos maririos merged commit 0fb3937 into Azure:feature/textanalytics/custom Sep 29, 2021
maririos pushed a commit to maririos/azure-sdk-for-net that referenced this pull request Oct 26, 2021
maririos added a commit that referenced this pull request Oct 26, 2021
* [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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants