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

.NET DPG LRO is making an extra GET call to the resource URI, breaking documented response schema #29347

Closed
heaths opened this issue Jun 16, 2022 · 5 comments
Labels
Client This issue points to a problem in the data-plane of the library. CodeGen Issues that relate to code generation Cognitive - Language
Milestone

Comments

@heaths
Copy link
Member

heaths commented Jun 16, 2022

For Sample5_ImportProject.cs in PR #29340, the ImportProject and Train LROs are working as expected based on the session file, but the DeployProject has an extra GET and I can't tell where it's coming from. Specifically, the initial LRO request indeed returns a 202 with an Operation-Location to a status monitor:

      "StatusCode": 202,
      "ResponseHeaders": {
        "Access-Control-Expose-Headers": "*",
        "apim-request-id": "3d75422d-f5fe-4bcd-8cdc-c1b240e63a78",
        "Content-Length": "0",
        "Date": "Wed, 15 Jun 2022 01:02:17 GMT",
        "operation-location": "https://languagesdkresource.cognitiveservices.azure.com/language/authoring/analyze-conversations/projects/net-conv-367666463/deployments/production/jobs/ff2585da-967b-41dc-8f05-0a854689e260_637908480000000000?api-version=2022-05-01",
        "request-id": "3d75422d-f5fe-4bcd-8cdc-c1b240e63a78",
        "Strict-Transport-Security": "max-age=31536000; includeSubDomains; preload",
        "X-Content-Type-Options": "nosniff",
        "x-envoy-upstream-service-time": "101"
      },
      "ResponseBody": null

The next several requests are what you'd expect - status is running - until the final status update:

    {
      "RequestUri": "https://languagesdkresource.cognitiveservices.azure.com/language/authoring/analyze-conversations/projects/net-conv-367666463/deployments/production/jobs/ff2585da-967b-41dc-8f05-0a854689e260_637908480000000000?api-version=2022-05-01",
      "RequestMethod": "GET",
      "RequestHeaders": {
        "Ocp-Apim-Subscription-Key": "Sanitized",
        "User-Agent": "azsdk-net-AI.Language.Conversations/1.0.0-alpha.20220614.1 (.NET 6.0.5; Microsoft Windows 10.0.22000)",
        "x-ms-client-request-id": "ccc3e9c28c3c019344e0e2ad20829ff8",
        "x-ms-return-client-request-id": "true"
      },
      "RequestBody": null,
      "StatusCode": 200,
      "ResponseHeaders": {
        "Access-Control-Expose-Headers": "*",
        "apim-request-id": "eb1b0550-ca5f-4a97-a177-169f5fdaaad4",
        "Content-Length": "218",
        "Content-Type": "application/json; charset=utf-8",
        "Date": "Wed, 15 Jun 2022 01:02:22 GMT",
        "request-id": "eb1b0550-ca5f-4a97-a177-169f5fdaaad4",
        "Strict-Transport-Security": "max-age=31536000; includeSubDomains; preload",
        "X-Content-Type-Options": "nosniff",
        "x-envoy-upstream-service-time": "24"
      },
      "ResponseBody": {
        "jobId": "ff2585da-967b-41dc-8f05-0a854689e260_637908480000000000",
        "createdDateTime": "2022-06-15T01:02:17Z",
        "lastUpdatedDateTime": "2022-06-15T01:02:22Z",
        "expirationDateTime": "2022-06-22T01:02:17Z",
        "status": "succeeded"
      }
    },

But then immediately afterward, there's this extra GET which is what the Operation<BinaryData>.Value ultimately contains:

    {
      "RequestUri": "https://languagesdkresource.cognitiveservices.azure.com/language/authoring/analyze-conversations/projects/net-conv-367666463/deployments/production?api-version=2022-05-01",
      "RequestMethod": "GET",
      "RequestHeaders": {
        "Ocp-Apim-Subscription-Key": "Sanitized",
        "User-Agent": "azsdk-net-AI.Language.Conversations/1.0.0-alpha.20220614.1 (.NET 6.0.5; Microsoft Windows 10.0.22000)",
        "x-ms-client-request-id": "8c11e8cce5e682c8638a22cdb3087be4",
        "x-ms-return-client-request-id": "true"
      },
      "RequestBody": null,
      "StatusCode": 200,
      "ResponseHeaders": {
        "Access-Control-Expose-Headers": "*",
        "apim-request-id": "35de4d51-fcec-4df0-82ca-c06206d6f36f",
        "Content-Length": "281",
        "Content-Type": "application/json; charset=utf-8",
        "Date": "Wed, 15 Jun 2022 01:02:23 GMT",
        "request-id": "35de4d51-fcec-4df0-82ca-c06206d6f36f",
        "Strict-Transport-Security": "max-age=31536000; includeSubDomains; preload",
        "X-Content-Type-Options": "nosniff",
        "x-envoy-upstream-service-time": "22"
      },
      "ResponseBody": {
        "deploymentName": "production",
        "modelId": "Sample5-20220615T010216-aeac9f0032944941b024163bc3ebff89",
        "lastTrainedDateTime": "2022-06-15T01:02:16.6869195Z",
        "lastDeployedDateTime": "2022-06-15T01:02:22Z",
        "deploymentExpirationDate": "2023-10-28",
        "modelTrainingConfigVersion": "2022-05-01"
      }
    }

I can't tell from where this comes? Why is the LRO querying the actual resource? The generated LRO implementation uses OperationFinalStateVia.Location just like the other aforementioned LROs:

using HttpMessage message = CreateDeployProjectRequest(projectName, deploymentName, content, context);
return LowLevelOperationHelpers.ProcessMessage(_pipeline, message, ClientDiagnostics, "ConversationAnalysisProjectsClient.DeployProject", OperationFinalStateVia.Location, context, waitUntil);

@annelo-msft @AlexanderSher @chunyu3 ?
/cc @kristapratico @mshaban-msft @tg-msft @johanste

Some useful links:

Originally posted by @heaths in #29331 (comment)

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 16, 2022
@azure-sdk
Copy link
Collaborator

Label prediction was below confidence level 0.6 for Model:ServiceLabels: 'Storage:0.19023241,Azure.Core:0.1606443,Service Bus:0.11049142'

@heaths heaths added Client This issue points to a problem in the data-plane of the library. CodeGen Issues that relate to code generation Cognitive - Language DPG/RLC v1.0 labels Jun 16, 2022
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 16, 2022
@heaths heaths added this to the [2022] July milestone Jun 16, 2022
@annelo-msft annelo-msft added the blocking-release Blocks release label Jun 16, 2022
@heaths
Copy link
Member Author

heaths commented Jun 16, 2022

FWIW, I tagged the reference commit from my fork to keep this living as long as my fork does.

@heaths
Copy link
Member Author

heaths commented Jun 16, 2022

Investigating if Python's deploy_project poller is doing the same thing, @kristapratico noticed and pointed out that the initial DeployProject operation is doing a resource PUT but returns an Operation-Location anyway, so this could be that our .NET LRO is simply following the old resource LRO (RELO) pattern of using a status monitor but doing a final GET on the resource URI to which a PUT was issued.

IMO, if an Operation-Location header was returned, perhaps that final GET on the resource URI shouldn't be assumed.

@kristapratico did have a good idea for a workaround, though: since we're already transforming the swagger for autorest to provide the schema for the final object, we could update the resource PUT to include an artificial 200 response with the GET's response model: "#/definitions/ConversationalAnalysisAuthoringProjectDeployment".

@tg-msft @mikekistler what are your thoughts here?

@heaths
Copy link
Member Author

heaths commented Jun 17, 2022

Talking offline, we'll do @kristapratico's idea for the CLU 1.0 GA:

@kristapratico did have a good idea for a workaround, though: since we're already transforming the swagger for autorest to provide the schema for the final object, we could update the resource PUT to include an artificial 200 response with the GET's response model: "#/definitions/ConversationalAnalysisAuthoringProjectDeployment".

@annelo-msft I'll leave it up to you whether you want to keep this issue as a tracking issue for support of this (anti?) pattern.

@annelo-msft
Copy link
Member

Based on investigations by @AlexanderSher, @kristapratico, and @lmazuel (thank you all!), we believe the .NET implementation is the same as Python's implementation and is correct. We make the final GET request because the original request was a PUT, as implemented here: https://github.com/Azure/autorest.csharp/blob/feature/v3/src/assets/Generator.Shared/NextLinkOperationImplementation.cs#L196

Per @lmazuel:

I believe if CLU fills the resourceLocation, all your problems go away and you don’t need any options, see guidelines:
https://github.com/microsoft/api-guidelines/blob/8465bf242ad1743beb49caa90a5a36c9190dc648/Guidelines.md#target-resource-location

Since the .NET implementation is correct, we will not make changes to the generator or shared source types.

Repository owner moved this from In Progress to Done in Azure SDK for Cognitive Services - Language Jun 18, 2022
@annelo-msft annelo-msft removed the blocking-release Blocks release label Jun 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Mar 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. CodeGen Issues that relate to code generation Cognitive - Language
Projects
Development

No branches or pull requests

3 participants