chore: Update Azure OpenAI types #141
Conversation
There was a problem hiding this comment.
Aside from the below problems, the type names are super long now and spread across so many files that it's hard to navigate them.
On the plus side, users seem to have more fields accessible to them now.
Maybe if we delete some of the weird looking things, or at least not export them publicly, this might be good enough?
| * A specific representation of configurable options for Azure Cosmos DB when using it as an Azure OpenAI chat | ||
| * extension. | ||
| */ | ||
| export type AzureOpenAiAzureCosmosDBChatExtensionConfiguration = |
There was a problem hiding this comment.
We should delete files that are not relevant for our client, like these cosmos-db files are. I am marking more files below.
There was a problem hiding this comment.
Only the relevant chat completion types are currently exported from the package.
I am honestly not sure about deleting them manually, the next time we update the spec we have the same problem. We dont delete unused types in Orchestration as well.
@marikaner what do you think?
There was a problem hiding this comment.
I think the idea would be to add a && rm ... to the script, or exclude it from the export. For orchestration we probably need to do the same, as we otherwise publish unreleased and non-functional API.
There was a problem hiding this comment.
Right now we exclude the export of unused types from both the packages (see index.ts). So even though we publish everything, users will not be able to access these types using @sap-ai-sdk/foundation-models.
I did this manually for now, maybe having a script would be a better (which either deletes the files or adds exports to index.ts).
| * | ||
| * This is a generated file powered by the SAP Cloud SDK for JavaScript. | ||
| */ | ||
| import type { AzureOpenAiOnYourDataAuthenticationOptions } from './on-your-data-authentication-options.js'; |
There was a problem hiding this comment.
delete this and all other on-your-data
packages/foundation-models/src/azure-openai/azure-openai-chat-completion-response.ts
Show resolved
Hide resolved
| /** | ||
| * Translation or transcription response when response_format was json. | ||
| */ | ||
| export type AzureOpenAiAudioResponse = { |
There was a problem hiding this comment.
Used by transcriptions which we currently don't offer API for, thus effectively unused at the moment
| /** | ||
| * Representation of the 'AzureOpenAiChatCompletionRequestMessage' schema. | ||
| */ | ||
| export type AzureOpenAiChatCompletionRequestMessage = { |
There was a problem hiding this comment.
This type is incorrect, it should include the mandatory field content. @marikaner figured this is due to the discriminator in the spec, which doesn't work in our generator, and is investigating if it can be fixed on a short notice..
There was a problem hiding this comment.
Probably the same issue applies for AzureOpenAiChatCompletionRequestMessageContentPart
| * A specific representation of configurable options for Azure Search when using it as an Azure OpenAI chat | ||
| * extension. | ||
| */ | ||
| export type AzureOpenAiAzureSearchChatExtensionConfiguration = |
There was a problem hiding this comment.
I'm fine to merge this first.
- When feat: Add support for discriminator cloud-sdk-js#5012 comes in, we can do another iteration, but this PR already improved the current status.
- For unused types, I dont mind, if we keep them, as they are not public APIs anyways. We can also create a follow up for a more sophisticated way to delete, instead of manual jobs.
This reverts commit d4ecd6c.
packages/foundation-models/src/azure-openai/azure-openai-chat-client.test.ts
Outdated
Show resolved
Hide resolved
|
|
||
| allExportedTypes.forEach(exportedType => { | ||
| if ( | ||
| !allExportedIndex.find(nameInIndex => exportedType.name === nameInIndex) | ||
| ) { | ||
| if (exportedType.path.split(sep).join(posix.sep).match(schemaPathRegex)) { |
There was a problem hiding this comment.
@marikaner @shibeshduw I adjusted the check public api script here to not complaint if I have a type specifically in ./schema that is not exported from index.ts. Logs a warning for those cases. We can remove it once we fix our generation or del unwanted types. Just FYI
There was a problem hiding this comment.
I linked this to AI/gen-ai-hub-sdk-js-backlog#115.
|
Please correct me: @deekshas8 |
|
@jjtang1985 Yes. |
Context
AI/gen-ai-hub-sdk-js-backlog#109.
Manual adjustments I had to make:
x-ms-examplessectionGenerate types using Azure Open AI spec version 2024-06-01
Definition of Done