Fix edge case for continue_final_message #36404
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
cc @Cyrilvallez @ArthurZucker for core maintainer review! |
|
Also added a clearer error message for the case described in #36440 - this should also be quite rare. |
| for content_block in reversed(final_message): | ||
| if "text" in content_block: | ||
| # Pick the last text block in the message (the first one we hit while iterating in reverse) | ||
| final_message = content_block["text"] | ||
| break | ||
| else: | ||
| raise ValueError( | ||
| "continue_final_message is set but we could not find any text to continue" | ||
| "in the final message!" | ||
| ) |
There was a problem hiding this comment.
We should not need to iterate here as we are either breaking or raising on first iter no?
There was a problem hiding this comment.
This was actually a massive bug! I think the only reason it didn't cause tests to fail was because of the faulty commit you mentioned below.
The else block is incorrectly indented - this should be a for/else loop, so the error is only raised if no blocks have a text field.
|
Also, please make sure to rebase to properly trigger tests as this one also has its branch pointing just after the faulty commit! |
Rocketknight1
force-pushed
the
fix-continue-final-message-edge-case
branch
from
4c05a70 to
9d0467a
Compare
Rocketknight1
force-pushed
the
fix-continue-final-message-edge-case
branch
from
9d0467a to
7b4b5da
Compare
* Fix edge case for continue_final_message * lstrip() correctly * Add regression test * Add a clearer error message when the final message is not present * Add a clearer error message when the final message is not present * Fix massive bug!
* Fix edge case for continue_final_message * lstrip() correctly * Add regression test * Add a clearer error message when the final message is not present * Add a clearer error message when the final message is not present * Fix massive bug!
The code for
continue_final_messageinapply_chat_templatefails in the following edge case:This causes the template to incorrectly identify the previous message as the final message and strip off a big chunk of the conversation after this point. This is a fairly uncommon edge case, but it's worth fixing nonetheless! cc @TK-21st
This PR handles things more elegantly without a try-except block, and also improves handling when assistant messages contain text/image blocks (which is rare right now, but will probably become more common in future)
Fixes #35433
Fixes #36440