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.

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

Sap generative ai hub #737

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

mbrner
Copy link

@mbrner mbrner commented Jan 9, 2024

This PR is to add the SAP Generative AI Hub as a model provider (reopened from #728).

The SAP Generative AI Hub provides a proxy for commercial models such as OpenAI and the ability to provide your own models. OAuth is always used for authentication, i.e. if you use OpenAI models via the Generative AI Hub, you do not need an API key, but the OAuth token.

In this PR, I have slightly modified the OpenAI class and introduced protected methods for determining the URLs and for generating the request headers. Based on the modified class, I added a SAPGenAI class that adapts the two methods for the Gen AI Hub.

ToDos:

  • Check how to adjust config validation, because the GenAI Hub's azure openai/apiType does not require engine + apiKeyand the openai type does not require apiKey.

Copy link

netlify bot commented Jan 9, 2024

Deploy Preview for continuedev canceled.

Name Link
🔨 Latest commit dc80fda
🔍 Latest deploy log https://app.netlify.com/sites/continuedev/deploys/659db310300819000873fc6b

@sestinj
Copy link
Contributor

sestinj commented Jan 12, 2024

@mbrner This looks really solid. Can we group together those options into a single interface like this?

interface SapGenAiOptions {
    // SAP Gen AI Core options
    resourceGroup?: string;
    authURL?: string;
    clientID?: string;
    clientSecret?: string;
}

Will help avoid needing to repeat all of these variable names repeatedly. We didn't do this with the Azure OpenAI options but I may go back and do that on my own—it seems like things will start to clutter without this.

Otherwise I'd consider this basically ready as long as it seems to be working on your side

@sestinj
Copy link
Contributor

sestinj commented Jan 12, 2024

There's a set of clauses for the models array inside of an allOf that has good example of how to conditionally require certain properties. In this case after grouping into the interface, you can just require the one.

A sample: https://github.com/continuedev/continue/blob/a3c6d04e6dfa88fa64a0a1b738509d930a9147f3/extensions/vscode/config_schema.json#L276C9-L288C11

@sestinj
Copy link
Contributor

sestinj commented Jan 22, 2024

Hi @mbrner, wanted to check in on this—happy to make these changes myself if it's easier, just let me know! Otherwise, since the PR is in draft state, I'll wait until I get a green-light from you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants