Feat/continue generate #361
Conversation
backend/app/usecases/chat.py
Outdated
| conversation.message_map[parent_id].children.append(message_id) # type: ignore | ||
|
|
||
| # TODO: Continueが空メッセージという仕様で良いか要検討 | ||
| # Empty messages for continuity purposes are not added. |
There was a problem hiding this comment.
- 空の判定はddbから読み取ったshould_continueから判別可能だと思ったのですが、ここで敢えて空の判定を挟んでいる理由を教えてください（打ち合わせ時に教えていただいたと思うのですが、忘れてしまいました。申し訳ないのですが、ここに記録残していただけると助かります。またそれをコード中のコメントで英語で記載しておいていただけると助かります）
仮に必要な場合は、bedrock claude chatは空メッセージをフロントから受け付けない仕様なので、空の判定で良いと思います！
- 空であることの判定は、websocket.pyに記載いただいているものと同様のもので良いと思いましたが、ここでtextかどうかも判定している理由はなんですか？
- new_messageの直前に、continueボタンによる分岐である旨をコメントで記載お願いします！
There was a problem hiding this comment.
空の判定はddbから読み取ったshould_continueから判別可能だと思ったのですが、ここで敢えて空の判定を挟んでいる理由を教えてください（打ち合わせ時に教えていただいたと思うのですが、忘れてしまいました。申し訳ないのですが、ここに記録残していただけると助かります。またそれをコード中のコメントで英語で記載しておいていただけると助かります）
仮にmax_tokensとなって続きがあったとしても、それを無視して別のリクエストを送るケースもあるかもという想定からです。should_continueに頼ってしまうとユーザの意図した動作とならない可能性があります。
仮に必要な場合は、bedrock claude chatは空メッセージをフロントから受け付けない仕様なので、空の判定で良いと思います！
了解です。
空であることの判定は、websocket.pyに記載いただいているものと同様のもので良いと思いましたが、ここでtextかどうかも判定している理由はなんですか？
単にtextでない場合の仕様をしっかり読み解けておらず、imageの場合にbodyが空の場合があるのかな？という想定からです。content_tyepが"image"の場合もbodyが空という状況はない感じでしょうか？
new_messageの直前に、continueボタンによる分岐である旨をコメントで記載お願いします！
分岐の直前にコメントで入れておきます。
There was a problem hiding this comment.
仮にmax_tokensとなって続きがあったとしても、それを無視して別のリクエストを送るケースもあるかもという想定からです。should_continueに頼ってしまうとユーザの意図した動作とならない可能性があります。
ありがとうございます、理解しました！
単にtextでない場合の仕様をしっかり読み解けておらず、imageの場合にbodyが空の場合があるのかな？という想定からです。content_tyepが"image"の場合もbodyが空という状況はない感じでしょうか？
別途slackでの議論の通りなのですが、空文字に意味を持たせたくないので、項目追加でお願いします！下記のようにChatInput (BE)と、PostMessageRequest (FE) に追加すればOKだと思います！
class ChatInput(BaseSchema):
conversation_id: str
message: MessageInput
bot_id: str | None = Field(None)
+ continue_generate: boolexport type PostMessageRequest = {
conversationId?: string;
message: MessageContent & {
parentMessageId: null | string;
};
botId?: string;
+ continueGenerate: bool
};| if chat_input.message.content[0].body != "": | ||
| messages.append(chat_input.message) # type: ignore | ||
| else: | ||
| if messages[-1].role == "assistant": |
There was a problem hiding this comment.
- assisntatかどうかの判別をあえて入れている理由は何でしょうか？（空メッセージでない = 続きが必要 = 自動的に最後のメッセージはassistant）
- 関数の責務を考慮すると、こちらではなくprepare_conversationで処理したほうが可読性が高いと思うので、strip処理はそちらに記載お願いします！
There was a problem hiding this comment.
assisntatかどうかの判別をあえて入れている理由は何でしょうか？（空メッセージでない = 続きが必要 = 自動的に最後のメッセージはassistant）
ここでtrim処理をしなければならないメッセージは必ずassistantの継続メッセージのみとなるため、安全のため(なんらかの仕様で空メッセージが入ってきた場合を考慮して)入れているだけなので、不要かもですね。
関数の責務を考慮すると、こちらではなくprepare_conversationで処理したほうが可読性が高いと思うので、strip処理はそちらに記載お願いします！
となると、前段で行なっているtrace_to_root処理をprepare_conversationで行うことになり冗長な動きになるか、messagesをprepare_conversationで作るとした場合、process_chat_inputで行なっている他の処理とのからみもあり大改修になりそうですが、いかがしましょう。
There was a problem hiding this comment.
大改修になりそうですが、いかがしましょう。
すみません、確かにtrace_to_root後に適用する必要がありますね。ご指摘ありがとうございます！
意図としては、この部分で細かな前処理をなるべく避けたい、というのが意図になります。例えば保存する前にtrimするというのはいかがでしょうか（問題なく出力できるかの簡単な検証は必要だと思いますが）。
ストリーム処理の場合、backend.app.stream.pyのAnthropicStreamHandler、BedrockStreamHandlerの該当箇所を下記のように変更することで、保存前にtrimが可能です。
AnthropicStreamHandlerの場合:
class AnthropicStreamHandler(BaseStreamHandler):
"""Stream handler for Anthropic models."""
def run(self, args: dict):
...
if isinstance(event, ContentBlockDeltaEvent):
...
elif isinstance(event, MessageDeltaEvent):
...
elif isinstance(event, MessageStopEvent):
concatenated = "".join(completions)
metrics = event.model_dump()["amazon-bedrock-invocationMetrics"]
input_token_count = metrics.get("inputTokenCount")
output_token_count = metrics.get("outputTokenCount")
price = calculate_price(
self.model, input_token_count, output_token_count
)
response = self.on_stop(
OnStopInput(
+ full_token=concatenated.strip(),
stop_reason=stop_reason,
input_token_count=input_token_count,
output_token_count=output_token_count,
price=price,
)
)
yield response
else:
continue非ストリーム処理で利用されるbackend.app.usecases.chat.pyのchat()では、reply_txtをstripする方針が綺麗にシンプルに実装できるかなと思いましたが、いかがでしょうか？
backend/app/websocket.py
Outdated
| @@ -181,7 +181,18 @@ def process_chat_input( | |||
| node_id=chat_input.message.parent_message_id, | |||
| message_map=message_map, | |||
| ) | |||
| messages.append(chat_input.message) # type: ignore | |||
|
|
|||
| continueGenerate = False | |||
There was a problem hiding this comment.
仮にddbに永続化されたものだけで判定が不可能である場合、この変数(continueGenerate)はprepare_conversationで返したほうが良いかと思いました（タプルに追加する）。
また細かいですが、should_continueではなくあえて変数名を変えた理由は何でしょうか？
There was a problem hiding this comment.
仮にddbに永続化されたものだけで判定が不可能である場合、この変数(continueGenerate)はprepare_conversationで返したほうが良いかと思いました（タプルに追加する）。
これは↑の改修とすこし絡みそうなので相談させてください
また細かいですが、should_continueではなくあえて変数名を変えた理由は何でしょうか？
単に修正漏れです。こちらの変数をshould_continue作成前に作っていたためです。名前合わせておきます。
There was a problem hiding this comment.
上記のコメント（continue_generateをinputに追加・保存前にtrim）を反映した場合、下記のようにシンプルになるかと思いましたが、いかがでしょうか？
if not chat_input.continue_generate:
messages.append(chat_input.message)
args = compose_args(
...略...
def on_stop(arg: OnStopInput, **kwargs) -> None:
if chat_input.continue_generate:
# For continue generate
conversation.message_map[conversation.last_message_id].content[
0
].body += arg.full_token
else:
...略...There was a problem hiding this comment.
こちら、continue_generate導入に伴い上記の用に修正できるので対応します！
|
duplicated PR: #280 |
| // [Customize]インプットメッセージもMarkdown書式で整形表示できるよう修正 | ||
| <ChatMessageMarkdown | ||
| key={idx} | ||
| messageId={String(idx)}> | ||
| {content.body} | ||
| </ChatMessageMarkdown> | ||
| ); |
There was a problem hiding this comment.
自身の入力データはRawな状態で表示しておきたいです。
LLMのプロンプトはある種のコマンドだと思うので加工はせずに、入力した内容をそのまま表示させておきたいという意図です。
マークダウンとRawな入力値を切り替え表示できるとベターな気がしますが、一旦このPRでは対応なしにしておきたいです
There was a problem hiding this comment.
ファイルアップロード時に表示する関係でマークダウンにしたと思うんですが、ファイルの中身は基本的に非表示として、クリックしたら中身が見れるようなUXにしたいです（コードを読み進めて気づきました）
利用者としては、画面上でファイルをアップロードしたという事実だけがわかれば良くて、ファイルの中身は追加アクションで確認できる方がUXが良いかなと思っています（長いファイルだと、ファイルの中身が表示されて画面が占有されてしまうため）
| .file-name { | ||
| text-align: center; | ||
| max-width: 8rem; /* ファイル名の幅 */ | ||
| white-space: pre-wrap; /* 折り返しを有効にする */ | ||
| word-wrap: break-word; /* 長い単語の途中で折り返す */ | ||
| } | ||
|
|
||
| .code-container { | ||
| padding: 3px; | ||
| } | ||
| .code-header { | ||
| margin-left: 18px; | ||
| margin-top: 5px; | ||
| } |
There was a problem hiding this comment.
スタイルの設定は、tailwind CSSで行なってください。
| const sendContent = useCallback(() => { | ||
| const filesString = textFiles.length > 0 ? textFiles.map(file => `\`\`\`${getFileLanguage(file.name)}:${file.name}\n${file.content}\n\`\`\`\n\n`).join('') : undefined; |
There was a problem hiding this comment.
claudeはXMLタグに最適化されていますが、マークダウンでも精度的には問題ないでしょうか？
| const inputRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| const truncateFileName = (name: string, maxLength = 30) => { |
There was a problem hiding this comment.
関数はuseCallbackの利用をお願いしますmm
| const { disabledImageUpload, model, acceptMediaType } = useModel(); | ||
|
|
||
| const extendedAcceptMediaType = useMemo(() => { | ||
| return [...acceptMediaType, '.md', '.ts', '.js']; // 追加のメディアタイプをここに追加 |
There was a problem hiding this comment.
利用できる拡張子は、hooksかconstantsで管理したいです。
extensionToLanguageMapも同様です。
| <PiFileTextThin className="h-16 w-16 text-gray-500" /> | ||
| <div className="file-name">{truncateFileName(file.name)}</div> |
There was a problem hiding this comment.
少し細かいですが、アイコンの大きさはw-12 h-12、文字サイズtext-sm、アイコンと文字の色:text-grayでお願いします。
| ))} | ||
| </div> | ||
| )} | ||
| {(getShouldContinue() || messages.length > 1) && ( |
There was a problem hiding this comment.
Reactの慣習として、テンプレート部分で関数を実行しないので、修正をお願いします。
hooks側で関数を実行して、その実行結果をStateに設定する方法をよく使います。
|
PR分割につき一旦draftにさせてください! (こちらはmergeせずcloseする予定です) |
Issue #, if available:
Description of changes:
React.FragmentからChatMessageMarkdownに変更stop_reasonがmax_tokensだった場合、続きを生成ボタンを出すように修正By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.