[Azure] Introduce AzureOpenAI client #718

deyaaeldeen · 2024-03-14T20:21:58Z

I understand that this repository is auto-generated and my pull request may not be merged

Changes being requested

Add an Azure client similar to Python's.

To-dos:

Export it from the package
~~Add support for Azure-specific models~~
Add an example
Add tests

Additional context & links

rattrayalex

@athyuttamre @kristapratico WDYT?

src/lib/azure/azure.ts

rattrayalex · 2024-03-15T11:57:27Z

Thank you for putting this up @deyaaeldeen ! We're discussing internally as well.

src/lib/azure/azure.ts

kristapratico

Looks good :)

src/lib/azure/azure.ts

deyaaeldeen · 2024-03-18T23:59:17Z

@rattrayalex I need your help in exporting the Azure client. The index.ts module has the definition of the OpenAI client but I think ideally, that definition may need to live in its own module so that index.ts can export both OpenAI and AzureOpenAI. What do you think?

rattrayalex · 2024-03-21T01:57:00Z

We'll take a look at that soon @deyaaeldeen. It may have to wait til next week.

deyaaeldeen · 2024-04-03T06:45:58Z

@rattrayalex Thanks for taking a look! I wonder if there is anything that I can clarify or help with to move this forward?

rattrayalex · 2024-04-03T21:15:43Z

thanks for the bump, this slipped through the cracks. I've poked internally.

pstern-sl · 2024-04-08T21:15:31Z

Hi! I will take a look and test on my side, but in the interim can you elaborate on the change you are proposing regarding the modules design? (for instance, what are the tradeoffs and alternatives?)

rattrayalex

Is it possible to leave index.ts largely untouched, instead of moving code into openai.ts?

…ally created

deyaaeldeen · 2024-04-25T21:39:43Z

Is it possible to leave index.ts largely untouched, instead of moving code into openai.ts?

@rattrayalex done. Could you please take another look? hopefully we can merge it ASAP 😊

rattrayalex

Thanks for the iteration here! I think its moving in a good direction. Just a few comments from a quick skim, but we need to discuss internally a bit on Monday.

Hopefully @RobertCraigie can provide a more thorough review soon.

src/lib/azure/azure.ts

src/index.ts

rattrayalex · 2024-04-26T23:37:29Z

src/lib/azure/azure.ts

+ apiVersion = Core.readEnv('OPENAI_API_VERSION'),
+ endpoint,
+ deployment,
+ microsoftEntraTokenProvider,


In the Python client, do we use the term Entra?

No, but it is a recent rebrand: https://www.microsoft.com/security/business/identity-access/microsoft-entra-id

RobertCraigie · 2024-05-03T11:45:51Z

Hey we've moved the OpenAI class definition to a separate src/client.ts file so you can properly inherit from it now :)

this subtly broke some setups that we didn't have tests for

deyaaeldeen · 2024-05-03T13:18:03Z

@RobertCraigie Awesome! I updated the client in 780ddbb.

RobertCraigie · 2024-05-03T13:35:44Z

Unfortunately we just had to revert the client.ts file change because it broke in certain cases: #816

I tried putting the azure exports at the bottom of the index.ts file and that works in some environments but not all :/

what would you think about this being the main import?

import { AzureOpenAI } from 'openai/azure';

Note that we tried a bunch of different ways of structuring the separate index.ts / client.ts files and it does not seem possible to make that change in a compatible fashion so we'll be looking into splitting them up in the next major version.

RobertCraigie · 2024-05-03T14:14:10Z

Actually, you could define the azure client class at the bottom of the index.ts file instead, that'll result in the best end user experience

deyaaeldeen · 2024-05-03T14:43:32Z

@RobertCraigie Thanks for the feedback! I updated the PR accordingly.

RobertCraigie · 2024-05-03T18:52:29Z

src/index.ts

+ /**
+ * A function that returns a Microsoft Entra (formerly known as Azure Active Directory) access token, will be invoked on every request.
+ */
+ microsoftEntraTokenProvider?: (() => string) | undefined;


is not supporting the equivalent azure_ad_token as well like in the Python client an intentional mismatch? (I haven't read all the context)

in other words, would people be suprised they can't set the AZURE_OPENAI_AD_TOKEN environment variable and have it picked up here?

@deyaaeldeen in addition to Robert's comments, in the Python client I believe we call this azure_ad not microsoft_entra – is that correct? Can we keep the naming consistent and call it Azure Active Directory instead?

is not supporting the equivalent azure_ad_token as well like in the Python client an intentional mismatch?

Yes, I would like our customers to use the token provider first because it takes care of token refreshing. I can add azureAdToken in the future if it turns out to be needed.

Can we keep the naming consistent and call it Azure Active Directory instead?

Ok I made the update in 670165f and we can plan the switch to the Entra name in the next major version.

RobertCraigie

Code changes look good to me. We'll also want to update the README.md docs.

Let me know when the docs have been updated & I'll port these changes over to our side before merging.

RobertCraigie · 2024-05-03T18:59:00Z

tests/azure.test.ts

nit: would be ideal for this to be in tests/lib/azure.test.ts, these are good tests though :)

I moved it in 670165f.

rattrayalex

Thanks for your patience here!

Few things outstanding:

Entra -> AD, as noted elsewhere
Need to update the Azure section of the README (or add one, similar to Python)

rattrayalex · 2024-05-03T22:19:17Z

Ah sorry, @deyaaeldeen my mistake – we actually don't need the README updated, we've done that part on our end. Just the Entra -> Azure AD change would be great.

rattrayalex · 2024-05-05T16:03:32Z

This has been moved to #822, which merged, and will be released in #823.

Thank you very much for the contribution and partnership!

## Linked issues closes: #1497 (issue number) ## Details **BREAKING CHANGE** * Updated AssistantPlanner to use `@azure/openai-assistants` as the underlying client. This supports both OpenAI and Azure OpenAI assistants v1 endpoints. * Updated math bot to use Azure OpenAI Assistants API by default. *Note*: Should migrate to official `openai-node` library once azure support is checked in. Here's the [PR](openai/openai-node#718 ## Attestation Checklist - [x] My code follows the style guidelines of this project - I have checked for/fixed spelling, linting, and other errors - I have commented my code for clarity - I have made corresponding changes to the documentation (updating the doc strings in the code is sufficient) - My changes generate no new warnings - I have added tests that validates my changes, and provides sufficient test coverage. I have tested with: - Local testing - E2E testing in Teams - New and existing unit tests pass locally with my changes --------- Co-authored-by: Corina <[email protected]>

deyaaeldeen added 4 commits March 14, 2024 20:16

[Azure] Introduce AzureOpenAI client

e926f06

remove header

d636a85

fix AAD auth and minor edits

afa9c1c

edit error message

fc316c9

rattrayalex reviewed Mar 15, 2024

View reviewed changes

src/lib/azure/azure.ts Outdated Show resolved Hide resolved

rattrayalex requested a review from athyuttamre March 15, 2024 11:52

deyaaeldeen added 2 commits March 15, 2024 15:56

fix Entra name

5967188

Merge remote-tracking branch 'origin/master' into feature/azure-client

40f00dd

bterlson reviewed Mar 15, 2024

View reviewed changes

src/lib/azure/azure.ts Outdated Show resolved Hide resolved

kristapratico reviewed Mar 15, 2024

View reviewed changes

src/lib/azure/azure.ts Outdated Show resolved Hide resolved

src/lib/azure/azure.ts Outdated Show resolved Hide resolved

address feedback

abbafde

deyaaeldeen added 5 commits April 19, 2024 17:51

Export Azure client

66aa785

add a test suite

a3f3aa8

add entra tests

0097b89

Merge remote-tracking branch 'upstream/master' into feature/azure-client

4f1c7f5

fix retries tests

86ae97d

rattrayalex reviewed Apr 20, 2024

View reviewed changes

deyaaeldeen added 2 commits April 20, 2024 21:02

fix path forming and add a couple tests

3b53a94

update azure example

2248b29

deyaaeldeen marked this pull request as ready for review April 21, 2024 20:53

deyaaeldeen requested a review from a team as a code owner April 21, 2024 20:53

deyaaeldeen added 2 commits April 22, 2024 21:31

Merge upstream/master

f18831d

Merge remote-tracking branch 'upstream/master' into feature/azure-client

69258c0

singhk97 mentioned this pull request Apr 22, 2024

[JS] feat: AssistantsPlanner support for Azure Assistants API microsoft/teams-ai#1571

Merged

1 task

revert changes in index.ts and export AzureOpenAI after it is dynamic…

7bc09b4

…ally created

deyaaeldeen requested a review from rattrayalex April 25, 2024 21:30

Merge remote-tracking branch 'upstream/master' into feature/azure-client

2b209f2

rattrayalex reviewed Apr 26, 2024

View reviewed changes

address feedback

e0f126c

manekinekko mentioned this pull request Apr 28, 2024

TODOs for Build 2024 Azure-Samples/azure-openai-assistant-javascript#1

Closed

2 tasks

fix(vectorStores): correct uploadAndPoll method (openai#817)

d63f22c

deyaaeldeen and others added 2 commits May 3, 2024 13:12

Merge remote-tracking branch 'upstream/master' into feature/azure-client

780ddbb

fix(package): revert recent client file change

2171d9e

this subtly broke some setups that we didn't have tests for

Merge branch 'next' into feature/azure-client

82d0f0d

address feedback

8a45377

merge upstream/master

074b330

stainless-bot force-pushed the feature/azure-client branch from d0e806e to 074b330 Compare May 3, 2024 14:49

Fix lint + add block comments

9a7ddbc

RobertCraigie reviewed May 3, 2024

View reviewed changes

rattrayalex reviewed May 3, 2024

View reviewed changes

update feedback

670165f

rattrayalex closed this May 5, 2024

sarangan12 mentioned this pull request May 9, 2024

openai[minor]: Update OpenAI with Azure Specific Code langchain-ai/langchainjs#5323

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Azure] Introduce AzureOpenAI client #718

[Azure] Introduce AzureOpenAI client #718

deyaaeldeen commented Mar 14, 2024 •

edited

Loading

rattrayalex left a comment

rattrayalex commented Mar 15, 2024

kristapratico left a comment

deyaaeldeen commented Mar 18, 2024 •

edited

Loading

rattrayalex commented Mar 21, 2024

deyaaeldeen commented Apr 3, 2024

rattrayalex commented Apr 3, 2024

pstern-sl commented Apr 8, 2024

rattrayalex left a comment

deyaaeldeen commented Apr 25, 2024

rattrayalex left a comment

rattrayalex Apr 26, 2024

deyaaeldeen Apr 26, 2024

RobertCraigie commented May 3, 2024

deyaaeldeen commented May 3, 2024

RobertCraigie commented May 3, 2024

RobertCraigie commented May 3, 2024

deyaaeldeen commented May 3, 2024

RobertCraigie May 3, 2024

rattrayalex May 3, 2024

deyaaeldeen May 4, 2024

RobertCraigie left a comment

RobertCraigie May 3, 2024

deyaaeldeen May 4, 2024

rattrayalex left a comment

rattrayalex commented May 3, 2024

rattrayalex commented May 5, 2024 •

edited

Loading

[Azure] Introduce AzureOpenAI client #718

[Azure] Introduce AzureOpenAI client #718

Conversation

deyaaeldeen commented Mar 14, 2024 • edited Loading

Changes being requested

Additional context & links

rattrayalex left a comment

Choose a reason for hiding this comment

rattrayalex commented Mar 15, 2024

kristapratico left a comment

Choose a reason for hiding this comment

deyaaeldeen commented Mar 18, 2024 • edited Loading

rattrayalex commented Mar 21, 2024

deyaaeldeen commented Apr 3, 2024

rattrayalex commented Apr 3, 2024

pstern-sl commented Apr 8, 2024

rattrayalex left a comment

Choose a reason for hiding this comment

deyaaeldeen commented Apr 25, 2024

rattrayalex left a comment

Choose a reason for hiding this comment

rattrayalex Apr 26, 2024

Choose a reason for hiding this comment

deyaaeldeen Apr 26, 2024

Choose a reason for hiding this comment

RobertCraigie commented May 3, 2024

deyaaeldeen commented May 3, 2024

RobertCraigie commented May 3, 2024

RobertCraigie commented May 3, 2024

deyaaeldeen commented May 3, 2024

RobertCraigie May 3, 2024

Choose a reason for hiding this comment

rattrayalex May 3, 2024

Choose a reason for hiding this comment

deyaaeldeen May 4, 2024

Choose a reason for hiding this comment

RobertCraigie left a comment

Choose a reason for hiding this comment

RobertCraigie May 3, 2024

Choose a reason for hiding this comment

deyaaeldeen May 4, 2024

Choose a reason for hiding this comment

rattrayalex left a comment

Choose a reason for hiding this comment

rattrayalex commented May 3, 2024

rattrayalex commented May 5, 2024 • edited Loading

deyaaeldeen commented Mar 14, 2024 •

edited

Loading

deyaaeldeen commented Mar 18, 2024 •

edited

Loading

rattrayalex commented May 5, 2024 •

edited

Loading