Update openai spec to use access and usage #25856
Conversation
Next Steps to Merge✔️ All automated merging requirements have been met! Refer to step 4 in the PR workflow diagram (even if your PR is for data plane, not ARM). |
Swagger Validation Report
|
Swagger Generation Artifacts
|
Generated ApiView
|
|
From typespec-java, we'd like to have this configure for Java (on access=public on models) PS: when we have the audio.tsp, Java could also have @@access(Azure.OpenAI.AudioTranscriptionOptions, Access.public, "java");
@@access(Azure.OpenAI.AudioTranslationOptions, Access.public, "java");I can update Java after Pan's PR. @jpalvarezl Let me know your opinion. |
|
Thanks for this change! I will bring it up in the meeting tomorrow where we sync across emitted languages. We need to see what the implications for .NET, JavaScript and Go would be, should this change be merged. Also, for more context, we are approaching a new release of the Azure OpenAI SDKs, so there is a chance this will have to wait until we are done with the release. |
microsoft-github-policy-service
bot
added
no-recent-activity
and removed
no-recent-activity
labels
|
Any update? For Java, I'd really like to make above update to client.tsp: #25856 (comment) The reason is that start from typespec-java 0.9.0+, it supports |
| @@access(Azure.OpenAI.ImageSize, Access.public); | ||
|
|
||
| @@access(Azure.OpenAI.AzureCognitiveSearchIndexFieldMappingOptions, Access.public); | ||
| @@usage(Azure.OpenAI.AzureCognitiveSearchIndexFieldMappingOptions, Usage.input); |
There was a problem hiding this comment.
.NET can generate but failed if including
@@usage(Azure.OpenAI.AzureCognitiveSearchIndexFieldMappingOptions, Usage.input);
There was a problem hiding this comment.
To clarify, there appears to be two issues in C#:
-
If I add this line:
@@usage(Azure.OpenAI.AzureCognitiveSearchChatExtensionConfiguration, Usage.input);
The code generation fails completely. -
If I remove the previous line and add this other line:
@@usage(Azure.OpenAI.AzureCognitiveSearchIndexFieldMappingOptions, Usage.input);
The code generation succeeds, but generates aWriteserialization method for theAzureCognitiveSearchIndexFieldMappingOptionsclass. However, this class already has a customWritemethod, so the compiler complains about having two different definitions and fails to build.
There was a problem hiding this comment.
- This comes from you have customized the properties
SearchKeyas type propertyAzureKeyCredential. I'm curious why you want to store credential in a model? - Yeah if they are the same you could remove the one in customization class. That is why I introduce this change - to remove the customization code.
There was a problem hiding this comment.
- This model is an input, and the
SearchKeyproperty is used to pass the API key that is needed to connect to the target Azure Cognitive Search resource. Since users are already familiar withAzureKeyCredentialas way of handling API keys, it made sense to us to make this property be anAzureKeyCredentialitself. Let me double check with the .NET architects what they think about this approach. - You're right that since the generated code is identical to the custom code, we can simply remove the custom code and it would work. That being said, would it be more appropriate if the emitter realized that there is already a customization and not emit the generated code unless the customization is removed first? This would prevent the emitter from producing code that the compiler cannot build.
There was a problem hiding this comment.
- Yeah, that is something could be discussed.
- I don't think it would be a good practice. Think about we have an update of the method body someday, then the final composed code would still have the old body in customization, which is not we are expecting.
|
Go has no changes after applying the changes. @richardpark-msft |
|
Hi, @pshao25 can you rebase the PR to the latest version of spec. |
…into feature/AccessAndUsage
|
@deyaaeldeen Does the "enum member not supported" issue still exist with the latest ts emitter ? |
|
@mssfang @joseharriaga Any further comments? After this change, all you need to do in azure-sdk-for-net is Azure/azure-sdk-for-net#39460 |
|
Sorry for dropping off like that. I had a couple of weeks re-assigned to a different work stream and then had my support shift. I would really want this PR merged, but there is one thing I would like us to do before moving forward. Let's only migrate the usages of This TSP definition is consumed by different language teams, so changing the visibility of models and operations should be a coordinated effort. In summary, I am asking that we remove all changes below line 19, please. Then let's merge the PR. Additional visibility changes should happen in separate PRs appropriately contextualised so we can communicate with all the stakeholders accordingly. |
I'm really only migrating the usages of @@internal and @@include, and only the models are touched, no operations. Not sure why need to remove 19. I'm just changing Or if you are saying 20 which not exists before, that is because |
|
@pshao25 , sorry for the misunderstanding. The current state of main only contains the following: import "@azure-tools/typespec-client-generator-core";
import "./main.tsp";
using Azure.ClientGenerator.Core;
// Azure-specific long-running operations should be treated as implementation details that are wrapped into
// appropriately merged public surface.
@@internal(Azure.OpenAI.beginAzureBatchImageGeneration);
@@internal(Azure.OpenAI.getAzureBatchImageGenerationOperationStatus);
// Azure-specific Chat Completions with extensions should be handled by clients as a conditional selection within the
// shared Chat Completions route, with the selection gated by the presence or non-presence of additional child
// configuration options on the request payload options model.
@@internal(Azure.OpenAI.getChatCompletionsWithAzureExtensions);
// Some models from routes with suppressed visibility are still desired for custom public surface.
@@include(Azure.OpenAI.ImageGenerationOptions);
@@include(Azure.OpenAI.ImageLocation);
@@include(Azure.OpenAI.ImageGenerations);I am suggesting we only migrate what we have on @@access(Azure.OpenAI.ImageSize, Access.public);
@@access(Azure.OpenAI.AzureCognitiveSearchIndexFieldMappingOptions, Access.public);
@@usage(Azure.OpenAI.AzureCognitiveSearchIndexFieldMappingOptions, Usage.input);
@@access(Azure.OpenAI.AzureCognitiveSearchQueryType, Access.public);
@@usage(Azure.OpenAI.AzureCognitiveSearchQueryType, Usage.input);
@@access(Azure.OpenAI.AzureCognitiveSearchChatExtensionConfiguration, Access.public);
@@usage(Azure.OpenAI.AzureCognitiveSearchChatExtensionConfiguration, Usage.input);So we can address the changes from line |
|
@qiaozha I can confirm this is still an issue with the TS emitter. The version used is 0.17.1. |
|
@jpalvarezl seems you misunderstand my intension. Nothing in generated SDK should change after this PR. |
|
@pshao25 , thanks for clarifying! I undertand that you want to replace the old However, I am seeing this PR adding new entries for changing the visibility of:
I would like to not add these new entries in the |
|
After our conversation offline, I understand why these new additions are added. Thank you @pshao25 for the clarification. Edit: I will open up this discussion as well to other people generating from these files to get gauge better how these changes in visibility will impact us. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Synced with all language owners. .net: will apply Azure/azure-sdk-for-net#39460 to fill the gap. |
|
@pshao25 @joseharriaga @pshao25 Would you mind having another look and merging this PR if no other concerns are left? |
Co-authored-by: Shawn Fang <[email protected]>
|
Turn out Mike already merged the changes before this PR to adopt We can close this PR if it is fine. |
I don't believe that produces correct SDK. Let's use the change in this PR. |
|
Swagger pipeline restarted successfully, please wait for status update in this comment. |
* Update openai spec to use access and usage * update * Update specification/cognitiveservices/OpenAI.Inference/client.tsp Co-authored-by: Shawn Fang <[email protected]> --------- Co-authored-by: Shawn Fang <[email protected]>
Choose a PR Template
Switch to "Preview" on this description then select one of the choices below.
Click here to open a PR for a Data Plane API.
Click here to open a PR for a Control Plane (ARM) API.