aws-samples / bedrock-claude-chat

AWS-native chatbot using Bedrock + Claude (+Mistral)
MIT No Attribution
687 stars 230 forks source link

Feat/continue to generate #401

Closed satoxiw closed 4 days ago

satoxiw commented 5 days ago

Issue #, if available:

Description of changes:

361 のPRで行った"続きを生成"機能をこちらのPRとして分割しました。#361はマージさせず、別途含めているファイル添付も機能を見直し別途PRさせていただきます。

本PRでは、#361で指摘いただいたコメントについて対応しています。下記に#361でいただいたコメントについての対応状況です。

@statefb さんにいただいたご指摘

別途slackでの議論の通りなのですが、空文字に意味を持たせたくないので、項目追加でお願いします!下記のようにChatInput (BE)と、PostMessageRequest (FE) に追加すればOKだと思います!

こちらいただいた対応方法に従って実装を修正しています。ただし、MessageContentcontentを空配列として渡してしまうとValidatorでエラーとなってしまうので、content.bodyに空文字を入力して送信するようにしています。(ただし、受け側の実装では空文字で判断はしていません)

https://github.com/aws-samples/bedrock-claude-chat/compare/v1...satoxiw:bedrock-claude-chat:feat/continue-to-generate?expand=1#diff-b3824293c8f669210fac6969ca83a8a56eb7490d6a1be0ad7f78ef934da1961fR515-R535

ストリーム処理の場合、backend.app.stream.pyのAnthropicStreamHandler、BedrockStreamHandlerの該当箇所を下記のように変更することで、保存前にtrimが可能です。

ご指摘通りの箇所でtrimするように実装修正しました。出力は問題なさそうでした。

https://github.com/aws-samples/bedrock-claude-chat/compare/v1...satoxiw:bedrock-claude-chat:feat/continue-to-generate?expand=1#diff-4ad0d81f00181ba7e06ecf54027552ade1db64bd3bcfd403e9c98d743bfa36e3R98

https://github.com/aws-samples/bedrock-claude-chat/compare/v1...satoxiw:bedrock-claude-chat:feat/continue-to-generate?expand=1#diff-4ad0d81f00181ba7e06ecf54027552ade1db64bd3bcfd403e9c98d743bfa36e3R137

非ストリーム処理で利用されるbackend.app.usecases.chat.pyのchat()では、reply_txtをstripする方針が綺麗にシンプルに実装できるかなと思いましたが、いかがでしょうか?

こちら、そもそも非ストリーム実装が全くできておりませんでした。この後実装追加します。

上記のコメント(continue_generateをinputに追加・保存前にtrim)を反映した場合、下記のようにシンプルになるかと思いましたが、いかがでしょうか?

こちらご指摘の修正でシンプルになりました。

https://github.com/aws-samples/bedrock-claude-chat/compare/v1...satoxiw:bedrock-claude-chat:feat/continue-to-generate?expand=1#diff-30a5880a3990bc0943cc7fb5748e0612709c07a190140ef49c1cf7cc16f99289R184-R186

@wadabee さんにいただたご指摘

Reactの慣習として、テンプレート部分で関数を実行しないので、修正をお願いします。 hooks側で関数を実行して、その実行結果をStateに設定する方法をよく使います。

useEffect hook内でgetShouldContinue()を呼び出すように修正してみました。もう少し良い修正があるのか判断できませんでしたので、改めて修正箇所確認いただければと思います。

https://github.com/aws-samples/bedrock-claude-chat/compare/v1...satoxiw:bedrock-claude-chat:feat/continue-to-generate?expand=1#diff-4f4332c2118277ddc90f75e9489ace8c6be3e9c60c88165e58063e8f25aa4734R100-R107

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

statefb commented 5 days ago

MessageContentのcontentを空配列として渡してしまうとValidatorでエラーとなってしまう

これ、ちなみにどこのバリデーションで引っかかってる感じでしょうか?

satoxiw commented 5 days ago

MessageContentのcontentを空配列として渡してしまうとValidatorでエラーとなってしまう

これ、ちなみにどこのバリデーションで引っかかってる感じでしょうか?

どこだったか改めて確認(contentを空にしてリクエスト)してみたのですが、遭遇したエラーになりませんでした。別のエラーと間違っていたようでしたので、生成を続ける時にはcontentを空にしてリクエストするように修正します