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

expire azure token as needed #615

Merged
merged 1 commit into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 12 additions & 2 deletions packages/cli/src/azuretoken.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
import { AZURE_OPENAI_TOKEN_SCOPES } from "../../core/src/constants"

export async function createAzureToken(signal: AbortSignal): Promise<string> {
export interface AuthenticationToken {
token: string
expiresOnTimestamp: number
}

export async function createAzureToken(
signal: AbortSignal
): Promise<AuthenticationToken> {

Choose a reason for hiding this comment

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

The parameter signal in the function createAzureToken is missing a type annotation. Please add a type annotation to improve code readability and maintainability. 📚

generated by pr-review-commit missing_param_type

const { DefaultAzureCredential } = await import("@azure/identity")
const azureToken = await new DefaultAzureCredential().getToken(
AZURE_OPENAI_TOKEN_SCOPES.slice(),
{ abortSignal: signal }
)
return azureToken.token
return {
token: azureToken.token,
expiresOnTimestamp: azureToken.expiresOnTimestamp,
}
}
11 changes: 7 additions & 4 deletions packages/cli/src/nodehost.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import {
import { AbortSignalOptions, TraceOptions } from "../../core/src/trace"
import { logVerbose, unique } from "../../core/src/util"
import { parseModelIdentifier } from "../../core/src/models"
import { createAzureToken } from "./azuretoken"
import { AuthenticationToken, createAzureToken } from "./azuretoken"
import { LanguageModel } from "../../core/src/chat"
import { errorMessage } from "../../core/src/error"

Expand Down Expand Up @@ -144,7 +144,7 @@ export class NodeHost implements RuntimeHost {
}
clientLanguageModel: LanguageModel

private _azureToken: string
private _azureToken: AuthenticationToken
async getLanguageModelConfiguration(
modelId: string,
options?: { token?: boolean } & AbortSignalOptions & TraceOptions
Expand All @@ -158,10 +158,13 @@ export class NodeHost implements RuntimeHost {
!tok.token &&
tok.provider === MODEL_PROVIDER_AZURE
) {
if (!this._azureToken)
if (
!this._azureToken ||
this._azureToken.expiresOnTimestamp >= Date.now()
)

Choose a reason for hiding this comment

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

The comparison this._azureToken.expiresOnTimestamp >= Date.now() seems to be incorrect. It should be this._azureToken.expiresOnTimestamp <= Date.now() to check if the token has expired. 🕰️

generated by pr-review-commit incorrect_comparison

this._azureToken = await createAzureToken(signal)

Choose a reason for hiding this comment

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

The function createAzureToken is an async function but it's not being awaited here. This could lead to unexpected behavior. Please use await to ensure the promise is resolved before proceeding. 🔄

generated by pr-review-commit async_await

if (!this._azureToken) throw new Error("Azure token not available")
tok.token = "Bearer " + this._azureToken
tok.token = "Bearer " + this._azureToken.token
}
if (!tok && this.clientLanguageModel) {
return <LanguageModelConfiguration>{
Expand Down