-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix (ai)!: move image model settings into generateImage({providerOptions})
#6083
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
Changes from all commits
f6f2708
1f569a8
8f08e9a
76cf257
ac97072
ef20d08
3d7bc41
7454034
8361281
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,20 +110,27 @@ Only applicable for HTTP-based providers. | |
}): Promise<GenerateImageResult> { | ||
const { retry } = prepareRetries({ maxRetries: maxRetriesArg }); | ||
|
||
// extract maxImagesPerCall from providerOptions as it's not meant to be passed to `doGenerate()` | ||
const [ | ||
[providerName, { maxImagesPerCall, ...generateProviderOptions } = {}] = [], | ||
] = Object.entries(providerOptions ?? {}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm surprised that we don't know I also don't know if this change breaks a use case that I'm not aware of? |
||
|
||
// default to 1 if the model has not specified limits on | ||
// how many images can be generated in a single call | ||
const maxImagesPerCall = model.maxImagesPerCall ?? 1; | ||
const maxImagesPerCallWithDefault = | ||
(maxImagesPerCall as number) ?? model.maxImagesPerCall ?? 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably the main change. Before we had access to |
||
|
||
// parallelize calls to the model: | ||
const callCount = Math.ceil(n / maxImagesPerCall); | ||
const callCount = Math.ceil(n / maxImagesPerCallWithDefault); | ||
const callImageCounts = Array.from({ length: callCount }, (_, i) => { | ||
if (i < callCount - 1) { | ||
return maxImagesPerCall; | ||
return maxImagesPerCallWithDefault; | ||
} | ||
|
||
const remainder = n % maxImagesPerCall; | ||
return remainder === 0 ? maxImagesPerCall : remainder; | ||
const remainder = n % maxImagesPerCallWithDefault; | ||
return remainder === 0 ? maxImagesPerCallWithDefault : remainder; | ||
}); | ||
|
||
const results = await Promise.all( | ||
callImageCounts.map(async callImageCount => | ||
retry(() => | ||
|
@@ -135,7 +142,9 @@ Only applicable for HTTP-based providers. | |
size, | ||
aspectRatio, | ||
seed, | ||
providerOptions: providerOptions ?? {}, | ||
providerOptions: providerName | ||
? { [providerName]: generateProviderOptions } | ||
: {}, | ||
Comment on lines
+145
to
+147
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a use case where providerOptions has more than one key? If so this change would break it. My current change doesn't pass through provider options because we want to remove the |
||
}), | ||
), | ||
), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,10 +16,7 @@ import { BedrockChatModelId } from './bedrock-chat-options'; | |
import { BedrockEmbeddingModel } from './bedrock-embedding-model'; | ||
import { BedrockEmbeddingModelId } from './bedrock-embedding-options'; | ||
import { BedrockImageModel } from './bedrock-image-model'; | ||
import { | ||
BedrockImageModelId, | ||
BedrockImageSettings, | ||
} from './bedrock-image-settings'; | ||
import { BedrockImageModelId } from './bedrock-image-settings'; | ||
import { | ||
BedrockCredentials, | ||
createSigV4FetchFunction, | ||
|
@@ -85,15 +82,16 @@ export interface AmazonBedrockProvider extends ProviderV2 { | |
|
||
embedding(modelId: BedrockEmbeddingModelId): EmbeddingModelV2<string>; | ||
|
||
image( | ||
modelId: BedrockImageModelId, | ||
settings?: BedrockImageSettings, | ||
): ImageModelV2; | ||
/** | ||
Creates a model for image generation. | ||
@deprecated Use `imageModel` instead. | ||
*/ | ||
image(modelId: BedrockImageModelId): ImageModelV2; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
imageModel( | ||
modelId: BedrockImageModelId, | ||
settings?: BedrockImageSettings, | ||
): ImageModelV2; | ||
/** | ||
Creates a model for image generation. | ||
*/ | ||
imageModel(modelId: BedrockImageModelId): ImageModelV2; | ||
} | ||
|
||
/** | ||
|
@@ -173,11 +171,8 @@ export function createAmazonBedrock( | |
fetch: sigv4Fetch, | ||
}); | ||
|
||
const createImageModel = ( | ||
modelId: BedrockImageModelId, | ||
settings: BedrockImageSettings = {}, | ||
) => | ||
new BedrockImageModel(modelId, settings, { | ||
const createImageModel = (modelId: BedrockImageModelId) => | ||
new BedrockImageModel(modelId, { | ||
baseUrl: getBaseUrl, | ||
headers: options.headers ?? {}, | ||
fetch: sigv4Fetch, | ||
|
Uh oh!
There was an error while loading. Please reload this page.