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

feat(community): Permit bedrock application inference profiles #7822

Merged
merged 4 commits into from
Mar 17, 2025

Conversation

bioshazard
Copy link
Contributor

@bioshazard bioshazard commented Mar 10, 2025

Fixes #7809

Bedrock Chat Model did not have a way to invoke Application Inference Profiles. Solving this was complicated at first... how do you keep all that modelId-derived metadata parsing?

So I added a modelAlias field that simply overrides this.model to be used in the /invoke URL. It expects to be URL encoded. Worked great in my local testing. You can find me on Twitter @bios_hazard

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 10, 2025
Copy link

vercel bot commented Mar 10, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview Mar 17, 2025 4:04pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ⬜️ Ignored (Inspect) Mar 17, 2025 4:04pm

@dosubot dosubot bot added the auto:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs label Mar 10, 2025
@AllenFang
Copy link
Contributor

invoke approach is complicated indeed but this patch looks hacky and like a workaround. I will see what I can do when i have time unless langchain team is okay for this patch. 🙏

@bioshazard
Copy link
Contributor Author

bioshazard commented Mar 12, 2025

Thanks, I think it's actually pretty clean given the meta data is parsed from the model id. All that is unavailable in the inference id arn. Curious to hear how you could do it another way with that limitation. I expect you'd need to do an intermediate lookup or supply a second field as I have.

Copy link
Collaborator

@jacoblee93 jacoblee93 left a comment

Choose a reason for hiding this comment

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

Thanks! Small question

For example, "arn%3Aaws%3Abedrock%3Aus-east-1%3A1234567890%3Aapplication-inference-profile%2Fabcdefghi", will override this.model in final /invoke URL call.
Must still provide `model` as normal modelId to benefit from all the metadata.
*/
modelAlias?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we call this applicationInferenceProfile?

Copy link
Contributor Author

@bioshazard bioshazard Mar 13, 2025

Choose a reason for hiding this comment

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

Works for me. Figured to make the url override generic tho application inference profile is the only use case today. I'll push the change today if y'all don't beat me to it

Copy link
Contributor Author

@bioshazard bioshazard Mar 13, 2025

Choose a reason for hiding this comment

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

@jacoblee93 ok I pushed an update to rename as suggested. Still working fine in testing in my app. Thanks!

Copy link
Contributor Author

@bioshazard bioshazard Mar 17, 2025

Choose a reason for hiding this comment

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

@jacoblee93 friendly bump, hoping to stay w/ LangChain as we migrate to App Inf profiles. Gotta deliver this soon. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will try to get this in today thanks!

Copy link
Collaborator

@jacoblee93 jacoblee93 left a comment

Choose a reason for hiding this comment

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

Thanks! Though note to others who might find this that it is better to switch to Bedrock's newer Converse API instead in @langchain/aws

@dosubot dosubot bot added the lgtm PRs that are ready to be merged as-is label Mar 17, 2025
@jacoblee93 jacoblee93 changed the title #7809: Permit bedrock application inference profiles feat(community): Permit bedrock application inference profiles Mar 17, 2025
@bioshazard
Copy link
Contributor Author

Thanks! Though note to others who might find this that it is better to switch to Bedrock's newer Converse API instead in @langchain/aws

Does that package support App Inf profiles yet? I probably need to pitch the same PR as I use that lib for embeddings.

@jacoblee93 jacoblee93 merged commit bac72ef into langchain-ai:main Mar 17, 2025
32 checks passed
@jacoblee93
Copy link
Collaborator

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs lgtm PRs that are ready to be merged as-is size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants