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: Add sagemaker endpoint support #1267

Merged
merged 9 commits into from
May 18, 2023

Conversation

cmanou
Copy link
Contributor

@cmanou cmanou commented May 15, 2023

  • Kept it as close to the python implementation as possible
  • I have not implemented the credentials_profile_name arg, given the changes with the new aws-sdk v3 I am wondering if we should just allow passing in the full credential arg so people can use whatever aws credential provider they want.
  • I don't know how the integration test should work given it would need a sagemaker endpoint, this looks like it was also an issue for the python implementation as it is also missing the tests.

@vercel
Copy link

vercel bot commented May 15, 2023

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

Name Status Preview Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview May 18, 2023 11:52am

@jacoblee93
Copy link
Collaborator

jacoblee93 commented May 15, 2023

This looks really cool!

Could you add an integration test to demonstrate how to set up and use this? A comment with some details about how to set it up would be fine - we should expand it to a docs page later.

@jacoblee93 jacoblee93 self-assigned this May 15, 2023
@cmanou
Copy link
Contributor Author

cmanou commented May 16, 2023

@jacoblee93 have added the docs and integration test albeit skipped due to the need for a sagemaker endpoint that's accessible for ci to test.

@jacoblee93
Copy link
Collaborator

jacoblee93 commented May 16, 2023

@jacoblee93 have added the docs and integration test albeit skipped due to the need for a sagemaker endpoint that's accessible for ci to test.

Thanks! Made a few modifications:

  1. You don't need to skip the integration tests as they don't run as part of CI right now.
  2. We don't need to do the await import syntax anymore - just import it, make sure it's under a separate entrypoint, and add it under requiresOptionalDependency in langchain/scripts/create-entrypoints.js: https://github.com/hwchase17/langchainjs/pull/1267/files#diff-d541783a759c4660823be9d9adc2406939edbc517f876c28b18136abd4db6f86R160
  3. We try to put all docs code examples in the examples/ folder now so our linter and formatter can run on them.

Also moved a few args and types around - worked great overall with HuggingFace GPT-2 for me. Thank you!

@jacoblee93 jacoblee93 added the lgtm PRs that are ready to be merged as-is label May 16, 2023
@cmanou
Copy link
Contributor Author

cmanou commented May 18, 2023

@jacoblee93 thanks for making those changes. Is there anything else needed to get this over the line ?

@nfcampos nfcampos merged commit 6f6d60a into langchain-ai:main May 18, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm PRs that are ready to be merged as-is
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants