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.

Sign up for GitHub

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

💅 Update text2vec-azure-openai to utilize isAzure: true flag and mark resourceName + deploymentId as optional #196

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

💅 Update text2vec-azure-openai to utilize isAzure: true flag and mark resourceName + deploymentId as optional #196

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9184e62
Update `text2vec-azure-openai` to utilize `isAzure: true` flag and ma…
flipace Sep 18, 2024
9f072ca
Update GenerativeOpenAI to support new isAzure: true flag
flipace Sep 18, 2024
ec23daa
chore: adjust types for text2VecAzureOpenAI to allow no object to be …
flipace Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/collections/config/types/generative.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,9 @@ export type GenerativeOpenAIConfig = GenerativeOpenAIConfigBase & {
};

export type GenerativeAzureOpenAIConfig = GenerativeOpenAIConfigBase & {
resourceName: string;
deploymentId: string;
resourceName?: string;
deploymentId?: string;
isAzure?: true;
tsmith023 marked this conversation as resolved.
Show resolved Hide resolved
};

export type GenerativePaLMConfig = {
Expand Down
10 changes: 6 additions & 4 deletions src/collections/config/types/vectorizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,12 @@ export type Text2VecAWSConfig = {
export type Text2VecAzureOpenAIConfig = {
/** The base URL to use where API requests should go. */
baseURL?: string;
/** The deployment ID to use */
deploymentId: string;
/** The resource name to use. */
resourceName: string;
/** The deployment ID to use. If left empty, must be provided via X-Azure-Deployment-Id header */
deploymentId?: string;
/** The resource name to use. If left empty, must be provided via X-Azure-Resource-Name header */
resourceName?: string;
/** Will automatically be set to true. You don't need to set this manually. */
isAzure?: true;
Comment on lines +187 to +188
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is always true, does it still need the ? operator? If not, can we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the true is always set by the text2VecAzureOpenAI function internally, should not be necessary to be passed by the user - but due to how the types in general are structured I could not easily 1) remove it completely from the config object, nor 2) remove the optional operator, since then the user would be required to supply isAzure: true manually 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahhh, okay I see now. I think this makes sense if the user makes use of the .azureOpenAI method but part of the API is still to allow users to work with the raw types if they so wish. As such, if a user did:

generative: {
  name: 'generative-openai',
  config: {
    deploymentId: config.deploymentId,
    resourceName: config.resourceName,
     baseURL: config.baseURL,
  }
}

then the type system would allow it since isAzure is optional yet the runtime would interpret this as isAzure: undefined, which is a false-y value.

I like the idea of introducing the isAzure flag to the TS client but I think it may be better placed as a pure internal, e.g. not exposed in the user types, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love to not expose it - but I could not figure out how to make it work so that it is not marked as unexpected internally where i place it 😬

This part here is what I mean:

CleanShot 2024-10-07 at 11 02 14@2x

I wouldn't know how to tell TS here that this is an expected prop without adding a // @ts-expect-error 🤔

Copy link
Collaborator

@tsmith023 tsmith023 Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My thinking is that isAzure should be removed from the ...Config types and instead interpreted by the client's runtime itself depending on the name of the module. So this would most likely require the addition of generative-azure-openai, alongside text2vec-azure-openai, that is then parsed appropriately in the collection creation logic

There we'd have some boolean clauses to determine whether the module is an azure one, based on the name, and then inject isAzure: true into the config appropriately. IMO, this would be the most consistent for the client/server relationship as I'm sure there will be future refactoring of the server that changes this behaviour. Then, we'd only break the internal relationship rather than the public API

We already do something similar here, wdyt about extending this logic as described above?

If you'd rather not then that's fine, I can add it to my backlog 😁 Also, sorry for the spaghetti of the collection.create method, I've not had the chance to refactor it into a better structure 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it - this definitely makes more sense 👍 I didn't look into this part.

I'm drowning a bit in other work right now, but I should be able to look into this in more detail hopefully next week or so 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I get round to it this week, I'll ping you on here to let you know. Thanks for your help so far!

/** Whether to vectorize the collection name. */
vectorizeCollectionName?: boolean;
};
Expand Down
1 change: 1 addition & 0 deletions src/collections/configure/generative.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ export default {
return {
name: 'generative-openai',
config: {
isAzure: true,
deploymentId: config.deploymentId,
resourceName: config.resourceName,
baseURL: config.baseURL,
Expand Down
21 changes: 21 additions & 0 deletions src/collections/configure/unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,24 @@ describe('Unit testing of the vectorizer factory class', () => {
config: {
deploymentId: 'deployment-id',
resourceName: 'resource-name',
isAzure: true,
},
},
});
});

it('should create the correct Text2VecAzureOpenAIConfig type with just isAzure: true', () => {
const config = configure.vectorizer.text2VecAzureOpenAI({});
expect(config).toEqual<VectorConfigCreate<never, undefined, 'hnsw', 'text2vec-azure-openai'>>({
name: undefined,
vectorIndex: {
name: 'hnsw',
config: undefined,
},
vectorizer: {
name: 'text2vec-azure-openai',
config: {
isAzure: true,
tsmith023 marked this conversation as resolved.
Show resolved Hide resolved
},
},
});
Expand All @@ -623,6 +641,7 @@ describe('Unit testing of the vectorizer factory class', () => {
deploymentId: 'deployment-id',
resourceName: 'resource-name',
vectorizeCollectionName: true,
isAzure: true,
},
},
});
Expand Down Expand Up @@ -1242,6 +1261,7 @@ describe('Unit testing of the generative factory class', () => {
expect(config).toEqual<ModuleConfig<'generative-openai', GenerativeAzureOpenAIConfig>>({
name: 'generative-openai',
config: {
isAzure: true,
resourceName: 'resource-name',
deploymentId: 'deployment-id',
},
Expand All @@ -1262,6 +1282,7 @@ describe('Unit testing of the generative factory class', () => {
expect(config).toEqual<ModuleConfig<'generative-openai', GenerativeAzureOpenAIConfig>>({
name: 'generative-openai',
config: {
isAzure: true,
resourceName: 'resource-name',
deploymentId: 'deployment-id',
baseURL: 'base-url',
Expand Down
5 changes: 4 additions & 1 deletion src/collections/configure/vectorizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,10 @@ export const vectorizer = {
vectorIndexConfig,
vectorizerConfig: {
name: 'text2vec-azure-openai',
config,
config: {
...config,
isAzure: true,
},
},
});
},
Expand Down
Loading