Skip to content

Dall-E models code generation#35549

Merged
mssfang merged 7 commits intomainfrom
jpalvarezl/dalle_regen
Jun 21, 2023
Merged

Dall-E models code generation#35549
mssfang merged 7 commits intomainfrom
jpalvarezl/dalle_regen

Conversation

@jpalvarezl
Copy link
Member

@jpalvarezl jpalvarezl commented Jun 20, 2023

Code gen related changes:

  • Incorporating changes introduced in the spec for Dall-E (code gen)
  • This would allow us to re-enable pipeline checks for API changes (they were disabled here )

Manual changes:

  • Added azure-core-experimental dependency to main pom.xml file
  • Locked the serviceVersion value to 2023_05_15 for 2 streaming tests where we were getting 200 response status but no values when requesting for version 2023_06_01_PREVIEW
  • Homogenized environment variable names. Changed NON_AZURE_OPEN_AI_KEY -> NON_AZURE_OPENAI_KEY

*/
@Generated
@ServiceMethod(returns = ReturnType.LONG_RUNNING_OPERATION)
public SyncPoller<PollResult, ImageOperationStatus> beginStartGenerateImage(
Copy link
Contributor

Choose a reason for hiding this comment

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

should we consider to rename this method name.

"beginStart" sounds the same thing.

Might be "beginGenerateImage" since it is LRO and should have started with begin prefix.

Copy link
Member Author

@jpalvarezl jpalvarezl Jun 21, 2023

Choose a reason for hiding this comment

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

This is a downstream change from the TSP definition. Let's surface this feedback in the Wednesday meeting, if that works for you.

Choose a reason for hiding this comment

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

@jpalvarezl

There is something about the definition of this LRO in tsp that Java codegen being checking with author.
Azure/azure-rest-api-specs#24382 (comment)

And arch is checking on the terminal state.
https://github.com/Azure/azure-rest-api-specs/pull/24382/files#r1237679552

So, the LRO may not work as expected in runtime. If you do test it, please let me know the runtime log. Thanks in advance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do. I've made a work item around this. It might be a couple of days before we get to the actual unit testing of this. Will keep you posted.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jun 25, 2023

Choose a reason for hiding this comment

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

Srikanta did a test

The final result is

{
  "created": 1687461794,
  "expires": 1687548201,
  "id": "bc67228d-12ec-45c7-b73a-3b8486465dfe",
  "result": {
    "created": 1687461794,
    "data": [
      {
        "url": "###"
      },
...
    ]
  },
  "status": "succeeded"
}

So the tsp definition on the LRO is wrong.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jun 25, 2023

Choose a reason for hiding this comment

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

A related topic: we also need some automation that runs when we update the autorest-java and typespec-java, to re-generate the SDK that has a codegen CI include, so that the upgrade on codegen won't break these CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am currently experiencing issues with the LRO in a sync scenario. For some reason I get 2 consecutive messages with status notRunning which prevent the continuation of the request. Async works as expected:

Sync call JSON:

{
  "Entries": [
    {
      "RequestUri": "https://REDACTED/openai/images/generations:submit?api-version=2023-06-01-preview",
      "RequestMethod": "POST",
      "RequestHeaders": {
       // ...
      },
      "RequestBody": {
        "prompt": "A drawing of the Seattle skyline in the style of Van Gogh"
      },
      "StatusCode": 202,
      "ResponseHeaders": {
        // ...
      },
      "ResponseBody": {
        "id": "22ee7ca1-2ff3-4698-8ff9-94fb8e4e3c32",
        "status": "notRunning"
      }
    },
    {
      "RequestUri": "https://REDACTED/openai/operations/images/22ee7ca1-2ff3-4698-8ff9-94fb8e4e3c32?api-version=2023-06-01-preview",
      "RequestMethod": "GET",
      "RequestHeaders": {
        // ...
      },
      "RequestBody": null,
      "StatusCode": 200,
      "ResponseHeaders": {
        // ...
      },
      "ResponseBody": {
        "created": 1687797150,
        "id": "22ee7ca1-2ff3-4698-8ff9-94fb8e4e3c32",
        "status": "notRunning"
      }
    }
  ],
  "Variables": {}
}

The Async case:

{
  "Entries": [
    {
      "RequestUri": "https://REDACTED/openai/images/generations:submit?api-version=2023-06-01-preview",
      "RequestMethod": "POST",
      "RequestHeaders": {
        // ...
      },
      "RequestBody": {
        "prompt": "A drawing of the Seattle skyline in the style of Van Gogh"
      },
      "StatusCode": 202,
      "ResponseHeaders": {
        // ...
      },
      "ResponseBody": {
        "id": "92da53f9-7d06-4d89-8a90-8667757f49b9",
        "status": "notRunning"
      }
    },
    {
      "RequestUri": "https://REDACTED/openai/operations/images/92da53f9-7d06-4d89-8a90-8667757f49b9?api-version=2023-06-01-preview",
      "RequestMethod": "GET",
      "RequestHeaders": {
        // ...
      },
      "RequestBody": null,
      "StatusCode": 200,
      "ResponseHeaders": {
        // ...
      },
      "ResponseBody": {
        "created": 1687796853,
        "id": "92da53f9-7d06-4d89-8a90-8667757f49b9",
        "status": "running"
      }
    },
    {
      "RequestUri": "https://REDACTED/openai/operations/images/92da53f9-7d06-4d89-8a90-8667757f49b9?api-version=2023-06-01-preview",
      "RequestMethod": "GET",
      "RequestHeaders": {
        // ...
      },
      "RequestBody": null,
      "StatusCode": 200,
      "ResponseHeaders": {
        // ...
      },
      "ResponseBody": {
        "created": 1687796853,
        "id": "92da53f9-7d06-4d89-8a90-8667757f49b9",
        "status": "running"
      }
    },
    {
      "RequestUri": "https://REDACTED/openai/operations/images/92da53f9-7d06-4d89-8a90-8667757f49b9?api-version=2023-06-01-preview",
      "RequestMethod": "GET",
      "RequestHeaders": {
        // ...
      },
      "RequestBody": null,
      "StatusCode": 200,
      "ResponseHeaders": {
        // ...
      },
      "ResponseBody": {
        "created": 1687796853,
        "expires": 1687883259,
        "id": "92da53f9-7d06-4d89-8a90-8667757f49b9",
        "result": {
          "created": 1687796853,
          "data": [
            {
              "url": "REDACTED"
            }
          ]
        },
        "status": "succeeded"
      }
    }
  ],
  "Variables": {}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

As you can see in the sync scenario we never actually get to a running state. Could be an issue with the local setup. Here is the code for Async:

public Mono<ImageOperationResponse> generateImage(ImageGenerationOptions imageGenerationOptions) {
  RequestOptions requestOptions = new RequestOptions();
  BinaryData imageGenerationOptionsBinaryData = BinaryData.fromObject(imageGenerationOptions);
  return serviceClient.beginStartGenerateImageAsync(imageGenerationOptions, requestOptions)
                .last()
                .flatMap(it -> it.getFinalResult())
                .map(it -> it.toObject(ImageOperationResponse.class));
}

And for the sync version. (the commented out options exhibit the same behaviour.

RequestOptions requestOptions = new RequestOptions();
        return beginStartGenerateImage(BinaryData.fromObject(imageGenerationOptions), requestOptions)
// option 1
            .waitUntil(LongRunningOperationStatus.SUCCESSFULLY_COMPLETED).getValue()
// option 2
//            .getFinalResult()

// option 3
//            .waitForCompletion()
//            .getValue()
            .toObject(ImageOperationResponse.class);

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jun 27, 2023

Choose a reason for hiding this comment

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

I wasn't aware that we had sync-stack enabled in openai. There were still a bug in pageable in current 1.40.0 core (that would be fixed in next 1.41.0 core).

OK, good thing is that openai don't have a pageable :-)

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jun 27, 2023

Choose a reason for hiding this comment

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

For the problem with sync polling, I am suspicious of a possible bug in SimpleSyncPoller. Investigating.
Issue #35628. It seems current code would treat any unknown status as isComplete=true.

@samvaity saw one in purview as well. Maybe same cause.

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-ai-openai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants