-
Notifications
You must be signed in to change notification settings - Fork 48
refactor: standardize Observation base class #929
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
Changes from all commits
13609c2
6520808
a349a7b
3ff4cb9
4c1d809
7ce7a9b
d0ca50a
b7efb0d
a93a9e2
bded49d
262e2a5
f0aaea0
11d5495
b52ce10
2c218cf
b4a29fc
5c422be
c2cb27b
adc5da0
09cee6b
157140c
651f957
2412728
da0e0be
e4f3efd
353b3e9
85773de
01479d7
6285b85
6470d0a
2d12008
0b4b706
b252b15
21d2d56
9b1868a
c474405
d2dba7a
23bb43d
5df3c52
12b4d19
b71b350
0e712a9
1ffcf6b
0b67da3
d609bf1
2351225
b6ad774
dfc918f
38841be
7b5ff64
446c53a
e17c515
f224f2a
a4cfa37
d63ed09
ccda8ea
c0750b0
0cf66ad
2645aab
249f9fe
18fced3
851213b
45bff22
5b4cffd
5ba09ad
b7ca7a2
98cce06
8d65a9b
f24ab0a
49e8832
fd18203
84da675
19e5a7b
8c3753c
e05c35d
cd8807b
e62771b
38256bf
85b1f15
2008c4b
a19e454
4a564b6
4293eb1
d1684ec
2fab1dd
1d16b6f
2bf41e2
ece030e
c648e23
1d0e7f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ | |
| from pydantic import Field | ||
| from rich.text import Text | ||
|
|
||
| from openhands.sdk.llm.message import ImageContent, TextContent | ||
| from openhands.sdk.tool.tool import ( | ||
| Action, | ||
| Observation, | ||
|
|
@@ -46,20 +45,15 @@ def visualize(self) -> Text: | |
|
|
||
|
|
||
| class ThinkObservation(Observation): | ||
| """Observation returned after logging a thought.""" | ||
|
|
||
| content: str = Field( | ||
| default="Your thought has been logged.", description="Confirmation message." | ||
| ) | ||
|
|
||
| @property | ||
| def to_llm_content(self) -> Sequence[TextContent | ImageContent]: | ||
| return [TextContent(text=self.content)] | ||
| """ | ||
| Observation returned after logging a thought. | ||
| The ThinkAction itself contains the thought logged so no extra | ||
| fields are needed here. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔
It's true that ThinkAction contains what the LLM sent us, but from the perspective of the LLM, now it has no confirmation, nothing. Maybe that works, but idk, it is a change. |
||
| """ | ||
|
|
||
| @property | ||
| def visualize(self) -> Text: | ||
| """Return Rich Text representation - empty since action shows the thought.""" | ||
| # Don't duplicate the thought display - action already shows it | ||
| """Return an empty Text representation since the thought is in the action.""" | ||
| return Text() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should still keep the no-op since we don't want to display "your thought has been logged". |
||
|
|
||
|
|
||
|
|
@@ -81,7 +75,7 @@ def __call__( | |
| _: ThinkAction, | ||
| conversation: "BaseConversation | None" = None, # noqa: ARG002 | ||
| ) -> ThinkObservation: | ||
| return ThinkObservation() | ||
| return ThinkObservation.from_text(text="Your thought has been logged.") | ||
|
|
||
|
|
||
| class ThinkTool(ToolDefinition[ThinkAction, ThinkObservation]): | ||
|
|
||
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.
I wonder if maybe we still need these two methods? 🤔
There's at least a difference in behavior:
to_llm_content()re-sent the user message before, are we sure an empty message works well, or works the same?