-
Notifications
You must be signed in to change notification settings - Fork 123
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
SageMaker Endpoint Support #80
Conversation
packages/cdk/lib/construct/api.ts
Outdated
@@ -30,38 +31,55 @@ export class Api extends Construct { | |||
|
|||
const { userPool, table, idPool } = props; | |||
|
|||
// sagemaker | bedrock | openai |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openaiの記述が残っています
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました
packages/cdk/lib/construct/api.ts
Outdated
nodeModules: [ | ||
'@aws-sdk/client-bedrock-runtime', | ||
'@aws-sdk/client-sagemaker-runtime', | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StreamingResponseが新しい機能なのでSDKをバンドルしていると思うのですが、コードをReadableにするという観点で、Streamingじゃない方の関数にもバンドルする設定があった方が良いかなと思いました!
もしくは、コメントで補足をお願いします!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StreamingResponse が必要なのはこの関数だけで他は元々 Lambda にあるものを利用できここに書くと Bundle Size が大きくなるので記載してません。コメントで補足しておきます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
追加で、本質と外れていて申し訳ないのですが、統一感的に prompt_templates ディレクトリを prompt-templates or promptTemplates に改名していただいも良いでしょうか 🙏
docs/BEDROCK.md
Outdated
CDK Deploy 時のパラメータもしくは `packages/cdk/cdk.json` で Context として指定することでリージョン、モデル、プロンプトを変更することが可能です。 | ||
|
||
```bash | ||
npm run cdk:deploy -- -c modelRegion=<Region> -c modelName=<Model Name> -c promptTemplate=<prompt_template_file> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
めっちゃ細かいですが、統一感的に <Prompt Template File>
ですかね
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
また、これだけだとどういうふうにモデルを変更できるのか (指定するのか) わからないと思います。具体的に他のモデルを指定するコマンドの例があった方が良いと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました
@@ -118,7 +118,7 @@ export const batchCreateMessages = async ( | |||
const createdDate = Date.now(); | |||
const feedback = 'none'; | |||
const usecase = ''; | |||
const llmType = 'OpenAI'; | |||
const llmType = 'bedrock'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 👍
docs/SAGEMAKER.md
Outdated
|
||
大規模言語モデルとして Amazon SageMaker エンドポイントにデプロイされたモデルを利用することが可能です。 | ||
|
||
Text Generation Inference (TGI) を使用した Huggingface Container を使用した SageMaker Endpoint に対応しています。Falcon をデプロイする際の例は[こちら](https://github.com/aws-samples/sagemaker-hosting/blob/main/GenAI-Hosting/Large-Language-Model-Hosting/LLM-Streaming/Falcon-40b-and-7b/falcon-40b-and-7b-tgi-streaming/falcon-7b-streaming_tgi.ipynb)です。`hf_model_id` で別のモデル(例:`elyza/ELYZA-japanese-Llama-2-7b-instruct`) を指定すれば別のモデルを使用することが可能です。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
前の PR にもコメントしましたが、ここで Falcon にしている理由はなんでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TGI の公式 Notebook がこれだったからですが、Rinna のブログと Elyza の Notebook に変更しました。
docs/SAGEMAKER.md
Outdated
``` | ||
|
||
promptTemplate はプロンプトを構築するためのテンプレートを JSON にしたファイル名を指定します。 (例: `llama2.json`) | ||
プロンプトテンプレートの例は `prompt_templates` フォルダを参照してください。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このドキュメントの構成の大枠から相談させてください。説明の順序として以下のような感じに変更するのはいかがでしょうか?
- SageMaker Endpoint & TGI が使えることを説明
- Prompt Template 的に使えるモデルが限定的であることを説明
- 使えるモデルを列挙 (デプロイ可能な Notebook のリンク付きで)
- デプロイする際のコマンドを説明 + 具体例
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます、修正しました。
{ | ||
bedrock: bedrockApi, | ||
sagemaker: sagemakerApi, | ||
}[modelType] || bedrockApi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ナイスです 👍
const message = line.replace(/^data:/, ''); | ||
const token: string = JSON.parse(message).token.text || ''; | ||
if (!token.includes(pt.eos_token)) yield token; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data:
で始まって json が入っていて \n
で終わる、みたいな仕様ってどこかにドキュメントあったりしますか?これだけ見てもわからないので、コメントで一行欲しいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コメントを追加しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ナイスコメントです!:+1:
packages/cdk/lib/construct/api.ts
Outdated
const sagemakerPolicy = new PolicyStatement({ | ||
effect: Effect.ALLOW, | ||
actions: ['sagemaker:DescribeEndpoint', 'sagemaker:InvokeEndpoint'], | ||
resources: ['arn:aws:sagemaker:*:*:endpoint/' + modelName], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*
の部分は region と account だと思うのですが、これはそれぞれ指定できると思うので、*
ではなく具体的な値にしたいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!! 👍
Issue #, if available:
#72
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.