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

Jump to bottom

feat: Model Discovery #75

Merged
merged 30 commits into from
Aug 23, 2024
Merged

feat: Model Discovery #75

merged 30 commits into from
Aug 23, 2024

Conversation

MatKuhr
Copy link
Member

@MatKuhr MatKuhr commented Aug 18, 2024 •
edited
Loading

Context

Closes AI/gen-ai-hub-sdk-js-backlog#57

API Design

Variant 1:

client.chatCompletion(OpenAiModels.GPT_4o, prompt);
client.chatCompletion({ name: 'gpt-4o', type: 'chat', version: '0613'}, prompt);

Autocompletion is available for name and type.

Variant 2:

client.chatCompletion(OpenAiModels.GPT_4o(), prompt);
client.chatCompletion(OpenAiModels.GPT_4o('0613'), prompt);

Autocompletion is available for the version number.

Variant 3:

client.chatCompletion('gpt-4o', prompt);
client.chatCompletion({ name: 'gpt-4o', version: '0613' }, prompt);

Autocompletion is available for the name.

Decision: We choose alternative 3, as it is the most JS-like style. We can still choose to offer 1 or 2 in addition the future, if we see need for it.

@MatKuhr MatKuhr mentioned this pull request Aug 18, 2024
Closed
@MatKuhr MatKuhr marked this pull request as ready for review August 19, 2024 13:18
@tomfrenken
Copy link
Member

tomfrenken commented Aug 20, 2024 •
edited
Loading

When we talked about the model_params in the refinement, I had an idea in regards to your static variant.

What if instead of

client.chatCompletion(OpenAiModels.GPT_4o(), prompt);
client.chatCompletion(OpenAiModels.GPT_4o('0613'), prompt);

You instantiate the models first, and then use them to create a config with proper autocomplete, e.g.:

const gptModel = OpenAiModels.GPT_4o('0613')
const config = gptModel.createConfig(<model_params_with_autocomplete>)
client.chatCompletion(config, prompt);

or depending on the implementation:

client.chatCompletion(
   OpenAiModels
   .GPT_4o('0613')
   .createConfig(<<model_params_with_autocomplete>>), prompt);

tomfrenken
tomfrenken reviewed Aug 20, 2024
View reviewed changes
Copy link
Member

@tomfrenken tomfrenken left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall looks like a great improvement, generally, I would like to stick to async / await instead of .then().catch(), and keep function calls shorter by destructuring the APIs, instead of:

WayTooLongApiCallFromSomeApi.theOneFunctionIActuallyNeed() <--- I get Java PTSD flashbacks
const { theOneFunctionIActuallyNeed } = WayTooLongApiCallFromSomeApi <--- the destructure variant

Additionally, if there is a way to avoid the many any types, we should try to narrow them as much as we can.

Other than that just some general suggestions for the open TODOs 👍

MatKuhr
MatKuhr commented Aug 22, 2024
View reviewed changes
packages/gen-ai-hub/src/orchestration/orchestration-client.test.ts
@@ -28,8 +24,7 @@ describe('GenAiHubClient', () => {
});

it('calls chatCompletion with minimum configuration', async () => {
const request: GenAiHubCompletionParameters = {
deploymentConfiguration,
const request = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Follow up for making the constants usable with orchestration: https://github.tools.sap/AI/gen-ai-hub-sdk-js-backlog/issues/91
Follow up for adding convenience for model_params: https://github.tools.sap/AI/gen-ai-hub-sdk-js-backlog/issues/89#issuecomment-7440288

MatKuhr
MatKuhr commented Aug 22, 2024
View reviewed changes
packages/gen-ai-hub/src/client/openai/openai-client.ts
data: OpenAiChatCompletionParameters,
deploymentResolver?: DeploymentResolver,
Copy link
Member Author

Choose a reason for hiding this comment

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

Follow-up BLI to improve the deploymentResolver: https://github.tools.sap/AI/gen-ai-hub-sdk-js-backlog/issues/92

MatKuhr
MatKuhr commented Aug 22, 2024
View reviewed changes
packages/gen-ai-hub/src/client/openai/openai-client.ts
}
const llm =
typeof model === 'string' ? { name: model, version: 'latest' } : model;
const deployment = await resolveDeployment({
Copy link
Member Author

Choose a reason for hiding this comment

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

if resolver is a function it is currently ignored. See https://github.tools.sap/AI/gen-ai-hub-sdk-js-backlog/issues/92

MatKuhr
MatKuhr commented Aug 22, 2024
View reviewed changes
packages/gen-ai-hub/src/index.ts
@@ -1,17 +1,10 @@
export * from './client/index.js';
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as no checks fail, yes. I think the API check is not in place yet, but once it is this might cause problems, not sure. I'd suggest to fix it then if needed.

marikaner
marikaner previously approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@marikaner marikaner left a comment

Choose a reason for hiding this comment

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

LGTM, I left a few minor comments and answers to questions you asked.

tomfrenken
tomfrenken previously approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@tomfrenken tomfrenken left a comment

Choose a reason for hiding this comment

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

Unintentionally dismissed Marika's LGTM when I resolved one of her comments 😬.

LGTM

deekshas8
deekshas8 reviewed Aug 23, 2024
View reviewed changes
packages/gen-ai-hub/src/client/openai/openai-client.ts
apiVersion
},
data,
requestConfig
mergeRequestConfig(requestConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

[q] Aren't we calling merge twice? Once here and then again in executeRequest?

deekshas8
deekshas8 reviewed Aug 23, 2024
View reviewed changes
packages/gen-ai-hub/src/client/openai/openai-client.ts
'content-type': 'application/json'
},
params: { 'api-version': apiVersion },
...requestConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

here the requestConfig will entirely replace the headers and params keys.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this is intended. My thinking was that the open AI client should only mix in whatever is specific to the open ai client.

tomfrenken
tomfrenken approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@tomfrenken tomfrenken left a comment

Choose a reason for hiding this comment

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

LGTM

@MatKuhr MatKuhr merged commit 6d4b700 into main Aug 23, 2024
10 checks passed
@MatKuhr MatKuhr deleted the feat/model-discovery branch August 23, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deekshas8 deekshas8 deekshas8 left review comments

@tomfrenken tomfrenken tomfrenken approved these changes

@marikaner marikaner marikaner left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MatKuhr @tomfrenken @marikaner @deekshas8