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

[Text Analytics] Regenerate with 3.1-preview.4 #13933

Merged
merged 8 commits into from
Feb 24, 2021

Conversation

deyaaeldeen
Copy link
Member

@deyaaeldeen deyaaeldeen commented Feb 23, 2021

Regenerates the text analytics package with endpoint v3.1-preview 4 and updates the convenience layer and tests accordingly. There are breaking renamings for opinion mining but this is fine because it is still in beta. There are also some changes to both beginAnalyzeBatchActions and beginAnalyzeHealthcareEntities which are also still in beta.

Note that the list of healthcare relation types is currently outdated in the swagger and we are waiting on the service team to update it so I ditched that enum for now and replaced it with a string.

To make reviewing easier, filter out all .js and .json files (they are test recordings).

@deyaaeldeen
Copy link
Member Author

/azp run js - ai-text-analytics - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@witemple-msft witemple-msft left a comment

Choose a reason for hiding this comment

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

LGTM

@deyaaeldeen deyaaeldeen merged commit 4c76e35 into Azure:master Feb 24, 2021
@deyaaeldeen deyaaeldeen deleted the textanalytics-preview4 branch February 24, 2021 13:43
// @public
export interface TextAnalyticsActionErrorResult {
readonly error: TextAnalyticsError;
readonly failedOn: Date;
Copy link
Member

Choose a reason for hiding this comment

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

this is the equivalent of TextAnalyticsActionSuccessState.completedOn but when there is an error, right?
Is it common in JS to use failedOn instead of completedOn?

Copy link
Member Author

Choose a reason for hiding this comment

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

is there an example of an endpoint that returns the failure date and time of a failed action? I just thought using the word completed here is misleading. I am not even sure why the service returns this to begin with.

Copy link
Member

Choose a reason for hiding this comment

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

They all do. See

{
    "jobId": "a0647292-7daf-449f-8cfa-081f3670db84",
    "lastUpdateDateTime": "2021-03-08T14:54:37Z",
    "createdDateTime": "2021-03-08T14:54:36Z",
    "expirationDateTime": "2021-03-09T14:54:36Z",
    "status": "failed",
    "errors": [
        {
            "code": "InvalidRequest",
            "message": "Job task parameter value phi is not supported for model-version parameter for job task type KeyPhraseExtraction. Supported values latest,2020-07-01.",
            "target": "#/tasks/keyPhraseExtractionTasks/0"
        }
    ],
    "tasks": {
        "details": {
            "lastUpdateDateTime": "2021-03-08T14:54:37Z"
        },
        "completed": 0,
        "failed": 1,
        "inProgress": 0,
        "total": 1,
        "keyPhraseExtractionTasks": [
            {
                "lastUpdateDateTime": "2021-03-08T14:54:37.8747764Z",
                "state": "failed"
            }
        ]
    }
}

And to clarify, your property is mapping to this area, right?

"keyPhraseExtractionTasks": [
            {
                "lastUpdateDateTime": "2021-03-08T14:54:37.8747764Z",
                "state": "failed"
            }
        ]

Copy link
Member Author

Choose a reason for hiding this comment

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

@maririos I think you misunderstood my comment. I meant to ask if you have any example of endpoints outside TA that does something similar so I can go and look at what the JS SDK does for naming such date property.

And to clarify, your property is mapping to this area, right?

Yes!

Copy link
Member

Choose a reason for hiding this comment

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

Ahh I see, I thought in TA.
I currently can't think of any but maybe looking on the API view of other libraries in JS might give u a hint

*/
export interface OpinionSentiment extends SentenceOpinion {}
export interface AssessmentSentiment extends SentenceAssessment {}

/**
* A mined opinion object represents an opinion we've extracted from a sentence.
Copy link
Member

Choose a reason for hiding this comment

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

NIT: An opinion object....

ghost pushed a commit that referenced this pull request Mar 8, 2021
I can not remember why I took it out here: #13933. This PR adds the option `displayName` back to `beginAnalyzeBatchActions`.
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