-
Notifications
You must be signed in to change notification settings - Fork 748
feat(core): add thought_description for simplified progress display #2163
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
base: develop
Are you sure you want to change the base?
Changes from all commits
f8d1ac0
d95b68d
23bfc14
9b85466
e9be4b1
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 | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,12 +21,15 @@ | |||||||||||||||||||||
| from pydantic import BaseModel | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| from nat.builder.builder import Builder | ||||||||||||||||||||||
| from nat.builder.context import Context | ||||||||||||||||||||||
| from nat.builder.function import Function | ||||||||||||||||||||||
| from nat.builder.function import LambdaFunction | ||||||||||||||||||||||
| from nat.builder.function_info import FunctionInfo | ||||||||||||||||||||||
| from nat.builder.workflow_builder import WorkflowBuilder | ||||||||||||||||||||||
| from nat.cli.register_workflow import register_function | ||||||||||||||||||||||
| from nat.data_models.function import FunctionBaseConfig | ||||||||||||||||||||||
| from nat.data_models.intermediate_step import IntermediateStep | ||||||||||||||||||||||
| from nat.data_models.intermediate_step import IntermediateStepType | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| class DummyConfig(FunctionBaseConfig, name="dummy"): | ||||||||||||||||||||||
|
|
@@ -90,6 +93,70 @@ async def test_direct_create_with_lambda(): | |||||||||||||||||||||
| assert await fn_obj.ainvoke("test", to_type=str) == "test!" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| async def test_thought_description_included_in_function_start_metadata(): | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| A function configured with `thought_description` should attach it as metadata on the | ||||||||||||||||||||||
| FUNCTION_START step it emits, so the step adaptor can surface it as a friendly thought label. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| async with WorkflowBuilder() as builder: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fn_obj = await builder.add_function(name="test_function_with_thought", | ||||||||||||||||||||||
| config=LambdaFnConfig(thought_description="Searching the web")) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| captured_steps: list[IntermediateStep] = [] | ||||||||||||||||||||||
| Context.get().intermediate_step_manager.subscribe(captured_steps.append) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| assert await fn_obj.ainvoke("test", to_type=str) == "test!" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| start_steps = [s for s in captured_steps if s.event_type == IntermediateStepType.FUNCTION_START] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| assert start_steps, "Expected at least one FUNCTION_START step to have been emitted" | ||||||||||||||||||||||
| assert start_steps[-1].metadata == {"thought_description": "Searching the web"} | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| async def test_thought_description_absent_when_not_configured(): | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| When `thought_description` is left unset (the default), no metadata should be attached to the | ||||||||||||||||||||||
| FUNCTION_START step, matching the pre-existing behavior for functions without a thought label. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| async with WorkflowBuilder() as builder: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fn_obj = await builder.add_function(name="test_function_without_thought", config=LambdaFnConfig()) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| captured_steps: list[IntermediateStep] = [] | ||||||||||||||||||||||
| Context.get().intermediate_step_manager.subscribe(captured_steps.append) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| assert await fn_obj.ainvoke("test", to_type=str) == "test!" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| start_steps = [s for s in captured_steps if s.event_type == IntermediateStepType.FUNCTION_START] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| assert start_steps, "Expected at least one FUNCTION_START step to have been emitted" | ||||||||||||||||||||||
| assert start_steps[-1].metadata is None | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||||||||||||||||||
| async def test_thought_description_included_in_function_start_metadata_for_streaming(): | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| Same as `test_thought_description_included_in_function_start_metadata`, but for the `astream` path: | ||||||||||||||||||||||
| a function configured with `thought_description` should attach it as metadata on the FUNCTION_START | ||||||||||||||||||||||
| step it emits when invoked via streaming rather than `ainvoke`. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
|
Comment on lines
+138
to
+142
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. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Fix the docstring summary line. Line [139] ends with Proposed fix- Same as `test_thought_description_included_in_function_start_metadata`, but for the `astream` path:
+ Verify `thought_description` metadata on the `astream` path.As per coding guidelines: “The first line of docstrings must be a concise description ending with a period (Vale checks this).” 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||||||||||||
| async with WorkflowBuilder() as builder: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fn_obj = await builder.add_function(name="test_stream_function_with_thought", | ||||||||||||||||||||||
| config=LambdaStreamFnConfig(thought_description="Searching the web")) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| captured_steps: list[IntermediateStep] = [] | ||||||||||||||||||||||
| Context.get().intermediate_step_manager.subscribe(captured_steps.append) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| results = [result async for result in fn_obj.astream("test", to_type=str)] | ||||||||||||||||||||||
| assert results == ["test!"] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| start_steps = [s for s in captured_steps if s.event_type == IntermediateStepType.FUNCTION_START] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| assert start_steps, "Expected at least one FUNCTION_START step to have been emitted" | ||||||||||||||||||||||
| assert start_steps[-1].metadata == {"thought_description": "Searching the web"} | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| async def test_direct_create_with_class(): | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| class ClassFnConfig(FunctionBaseConfig, name="test_class"): | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
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.
Why is this called
thought_textand not consistent withthought_description?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.
thought_descriptionis the raw value configured onFunctionBaseConfig, essentially the "input".thought_textonResponseIntermediateStepis the rendered string actually shown in the UI, it's derived fromthought_descriptionviaStepAdaptor._get_thought_description(): falls back to a default like "Running function: " whenthought_descriptionisn't set, and always has a status suffix appended ("..." while running, "... completed" once finished). So it's never a direct copy of the config value, it seemed clearer to give it a distinct name (thought_text) rather than reusingthought_descriptionfor a value that's been transformed.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.
Right, but this now increases cognitive load. It's still a description. And now folks have to manually tie
description->text.Reduced orthogonality is a good thing.
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.
@GraceJiang0312 to be clear, I'm waiting for an update that addresses this. I do not believe we should have two different names where one is effectively the same as the other. Both are
thought_descriptions. There is no reason to introduce another name.