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

Adding a minimal Azure OpenAI Inference SDK. #1813

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

Conversation

jpalvarezl
Copy link
Member

@jpalvarezl jpalvarezl commented Sep 19, 2024

Sorry for not addressing the style checks yet. I am aware of them and will address them ASAP. I just wanted to have this PR opened before the Hackathon was over. Style checks have been addressed.

Contents of this PR

  • Minimal definition of ChatCompletions operation. Using as many server default values as possible.
  • Support for streamed responses
  • Examples using key credentials and AAD
  • ChatCompletionsClient can be used with Azure or non-Azure OpenAI service

Remaining TODOs from original PoC

  • Would need multipart/form-data support to add audio related operations (happy to contribute this, if that is an option. I have a bad (but working) implementation for this, in the original PoC)
  • Add additional fields to ChatCompletions models
  • Use code generation tools wherever possible

Edited on 01/10/2024

@heaths
Copy link
Member

heaths commented Sep 23, 2024

Please go through style errors. We don't want to increase technical debt, which is why they are build errors. I find it helps to run them locally before pushing more commits, especially since it keeps updating the PR.

On that note, I'm very busy at the moment trying to prepare crates we plan on releasing as betas soon. What timeframe do you have in mind for releasing an OpenAI SDK for Rust? It was not previously on our docket of support. /cc @RickWinter @ronniegeraghty

@heaths
Copy link
Member

heaths commented Sep 24, 2024

We're still working on some big changes with azure_core and TypeSpec-branded crates. It sounds as if this crate would have no official support yet - @ronniegeraghty will be contacting you internally - so we'd like to have this in a separate feature branch for now so we're not spending a lot of resources keeping it up to date until its ready for broad support from your team.

I'll do a cursory review about the overall structure and public APIs, but not in-depth at this time. Can you also create a separate feature branch e.g., feature/openai in the Azure/azure-sdk-for-rust repo and retarget your PR (click on the Edit button by the PR title) on that?

git fetch upstream feature/track2 # assumes this is the "upstream" repo
git push upstream FETCH_HEAD:feature/openai
git checkout jpalvarezl/aoai_from_track2 # check out your topic branch if not already
git rebase upstream/feature/openai
git push --force

Then in your PR on this site, retarget using the Edit button.

sdk/openai/inference/Cargo.toml Outdated Show resolved Hide resolved
sdk/openai/inference/examples/azure_chat_completions.rs Outdated Show resolved Hide resolved
sdk/openai/inference/src/auth/azure_key_credential.rs Outdated Show resolved Hide resolved
sdk/openai/inference/src/lib.rs Outdated Show resolved Hide resolved
use super::*;

#[derive(Clone, Debug, Default)]
pub struct OpenAIClientOptionsBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

You should also implement azure_core::ClientOptionsBuilder. See guidelines for details. We'll have a derive macro eventually to make this easier but aren't there yet.

Copy link
Member

Choose a reason for hiding this comment

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

Same is true for client method options.

sdk/openai/inference/src/clients/openai_client.rs Outdated Show resolved Hide resolved
}

impl BaseOpenAIClientMethods for AzureOpenAIClient {
fn base_url(&self, deployment_name: Option<&str>) -> azure_core::Result<Url> {
Copy link
Member

Choose a reason for hiding this comment

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

As a caller, I would wonder what the difference is between endpoint and base_url. Public API docs will be required before you can ship, but I would describe at least these two methods now.

async fn create_chat_completions(
&self,
deployment_name: impl AsRef<str>,
chat_completions_request: &CreateChatCompletionsRequest,
Copy link
Member

Choose a reason for hiding this comment

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

These should be client method options. See guidelines.

@jpalvarezl jpalvarezl force-pushed the jpalvarezl/aoai_from_track2 branch from f00dd19 to a82e472 Compare September 30, 2024 13:11
@heaths
Copy link
Member

heaths commented Sep 30, 2024

This is clearly going through a lot of development and iteration. I highly recommend building and testing locally before submitted. As it's not ready for review (see comments above, along with how frequently you're pushing commits) I'm marking this as a draft. When you think it's ready for a final review and have talked with @ronniegeraghty, please convert it back to a full PR.

@heaths heaths marked this pull request as draft September 30, 2024 23:18
@jpalvarezl
Copy link
Member Author

jpalvarezl commented Oct 2, 2024

This is clearly going through a lot of development and iteration. I highly recommend building and testing locally before submitted. As it's not ready for review (see comments above, along with how frequently you're pushing commits) I'm marking this as a draft. When you think it's ready for a final review and have talked with @ronniegeraghty, please convert it back to a full PR.

My apologies if it seemed like I was adding more changes to this PR. I tried following the steps you suggested to move the branch to this repository, but I must have done something wrong and ran into some issues which resulted on me ending up doing something else entirely by accident, causing a mess with the commit history. It seems like I rebased Azure/feature/track2, which was injected as a commit at what was HEAD at the time of trying. I will sort this out.

First of all, thank you for your early feedback. I have addressed all the items that I marked as resolve taking in your comments in full. Also, addressed the CI checks. The comments that are still unresolved, seemed a bit more involved, so I want to find the time to be able to dedicate my full attention to them.

I am waiting on being able to talk sync with @trrwilson to figure out how to best approach and move forward with this effort.

@heaths heaths changed the base branch from feature/track2 to main October 18, 2024 23:55
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.

2 participants