Use Agent Docstring and Module Docstring as Agent system prompt#585
Use Agent Docstring and Module Docstring as Agent system prompt#585
Conversation
| template, | ||
| "__system_prompt__", | ||
| textwrap.dedent(f""" | ||
| SYSTEM: You are a helpful LLM assistant named {template.__name__}. |
There was a problem hiding this comment.
Should we just remove this hard-coded default given the degradation it seems to have caused in #567 ? We want the system message to be fully modifiable by the user, so falling back to the module docstring in Template.define might be better.
There was a problem hiding this comment.
While updating it I realize that this might not work in notebook as notebook would not have module doc. For Agent, it's fine as Template can still use Agent's docstring, but not normal templates, so I used another prompt instead.
effectful/handlers/llm/template.py
Outdated
| """Build an Agent system prompt from module and class docstrings.""" | ||
| parts: list[str] = [] | ||
|
|
||
| mod = inspect.getmodule(cls) |
There was a problem hiding this comment.
This part with the module-level docstring should probably happen in Template.define, so that mod is the module of the Template and this covers non-method Templates as well.
There was a problem hiding this comment.
I moved it into Template.define
effectful/handlers/llm/template.py
Outdated
|
|
||
| mod = inspect.getmodule(cls) | ||
| if mod is not None and mod.__doc__: | ||
| parts.append(inspect.cleandoc(mod.__doc__)) |
There was a problem hiding this comment.
I'm not sure if it makes a difference for modules, but you might want to use inspect.getdoc here instead of mod.__doc__.
There was a problem hiding this comment.
I used inspect.getdoc now.
effectful/handlers/llm/template.py
Outdated
| if mod is not None and mod.__doc__: | ||
| parts.append(inspect.cleandoc(mod.__doc__)) | ||
|
|
||
| class_doc = cls.__dict__.get("__doc__") |
There was a problem hiding this comment.
If you use inspect.getdoc here instead of just looking at the __doc__ attribute, it will automatically go up the inheritance hierarchy to find the first non-empty class docstring. Note that that would in some cases be the docstring of Agent itself, which we may or may not want (or we may want to change the Agent docstring so that it is more amenable to being used this way).
There was a problem hiding this comment.
I updated to use inspect.getdoc here as well.
effectful/handlers/llm/template.py
Outdated
| class_doc = cls.__dict__.get("__doc__") | ||
| if class_doc is None or not inspect.cleandoc(class_doc): | ||
| raise ValueError( | ||
| "Agent subclasses must define a non-empty class docstring." | ||
| ) |
There was a problem hiding this comment.
I'm not sure we want this to be an error. As I wrote in one of my other comments, using inspect.getdoc rather than cls.__doc__ would look at the inheritance hierarchy of cls and get the first non-empty docstring, which would always include Agent as a fallback.
There was a problem hiding this comment.
I also updated it, as it has fallback, I dont need to throw ValueError anymore.
effectful/handlers/llm/template.py
Outdated
| __system_prompt__: str | ||
|
|
||
| @classmethod | ||
| def _build_system_prompt(cls) -> str: |
There was a problem hiding this comment.
I would just inline this logic into Agent.__init_subclass__ / Template.define so that it's not something that can be overridden in subclasses. There are already two ways of customizing the system prompt (docstrings and handling call_system), having a third is unnecessarily confusing.
| prop.__set_name__(cls, "__history__") | ||
| cls.__history__ = prop | ||
| if not hasattr(cls, "__system_prompt__"): | ||
| sp = functools.cached_property( |
There was a problem hiding this comment.
In this PR, __system_prompt__ is actually fixed at the class rather than instance level, despite being an instance-level cached_property. Are there cases where we might want __system_prompt__ to vary across instances of the same Agent class? If so, how?
There was a problem hiding this comment.
That's an interesting question. I think we might actually want to have agent of same class but behave slightly differently.
For example, agents Alice and Bob communicating with each other might want to know who they are. We could make Alice and Bob subclass. But that stop us from mass-producing these agents.
We could also move __system_prompt__ to be at init level or a property definable by user.
There was a problem hiding this comment.
Bound method Templates already have access to Agent instance-level data in user messages via self, so I'm not sure identifiers like "alice"/"bob" are the best example. It seems to me that something should only go in the system message if it satisfies some or ideally all of the following properties:
- Constant and immutable at an
Agentclass or instance level (so that the system message can't change during the lifetime of anAgentinstance) - Naturally associated with an
Agent,Templateor module rather than a data type orTool(so that we aren't duplicating structured input/output specifications already handled byEncodableorTool) - Identifiable mechanically from runtime introspection (so that we aren't making arbitrary internal choices for the system prompt that lead to repeats of Call_system should be more emphasized by Template docstring. #567 )
- Large or complicated (so that repeatedly encoding the same information across user messages would be too wasteful)
- Unconditionally useful for any call to any method
Template(so that the system prompt isn't inlining something that should be gated behind a tool call)
There are a number of things that are not instance-specific that satisfy these properties (e.g. module source code), but the only specifically instance-level things that come to mind immediately are:
- Annotated fields that are not explicitly accessed by any
Templateand are known somehow to be immutable e.g. atuple[Image, ...]field on adataclass. More generally I guess you could imagine anAnnotationfor use onAgentclass definitions that asserts that some instance-level field should be appended to the system message of an instance. This seems likely to be especially useful for non-textual data. - Compactified message history - as with ordinary coding agents or chatbots, once an
Agent's__history__gets to be too long for the underlying model, it has to be compressed somehow to continue using it. One way to do this is by having the model summarize it and append the resulting constant-size summary to the system message before clearing__history__.
Both of those are speculative and out of scope for this PR, though.
I'd say no, since we don't want the system message to change once it's been set.
Module before
This seems fine, not all regular modules have docstrings either. |
I think this should just work as expected with the design in this PR (because |
…ompt construction to template
I think it might, but we don't know until we have cases that break because of wrong order, so keeping this until then |
Got it. It's fine for the agent. But I think |
I added test to make sure that this is true. |
Maybe I misunderstood, but I interpreted your earlier comment about this to be about histories rather than tools? I don't think we'd want to change the current tool collection behavior to make the new test you added pass. |
effectful/handlers/llm/template.py
Outdated
|
|
||
| # Collect tools as methods on Agent instances in context | ||
| elif isinstance(obj, Agent): | ||
| elif include_agent_instance_tools and isinstance(obj, Agent): |
There was a problem hiding this comment.
This should not be changed, the existing behavior seems correct to me.
tests/test_handlers_llm_template.py
Outdated
| return standalone, captured_agent | ||
|
|
||
| standalone, _ = make_template() | ||
| assert "helper" not in standalone.tools |
There was a problem hiding this comment.
The only problem I see here is that the name should be "helper_agent.helper" or "captured_agent.helper" rather than "helper". Otherwise I don't think this test should pass, the Agent's methods should be visible to standalone.
There was a problem hiding this comment.
Understood. I removed this test.
tests/test_handlers_llm_template.py
Outdated
| assert MissingDocAgent.__doc__ is None | ||
| prompt = MissingDocAgent().__system_prompt__ | ||
| assert prompt | ||
| assert "persistent LLM message history" in prompt |
There was a problem hiding this comment.
This hardcoded substring of the current Agent.__doc__ will cause the test to break immediately upon changes to Agent.__doc__.
tests/test_handlers_llm_template.py
Outdated
| assert_single_system_message_first(mock.received_messages[0]) | ||
| assert ( | ||
| mock.received_messages[0][0]["content"] | ||
| == "You are a helpful assistant, you need to follow user's instruction" |
There was a problem hiding this comment.
This hardcoded value will break this test immediately upon any changes to default behavior of call_system.
tests/test_handlers_llm_template.py
Outdated
| assert agent_doc is not None | ||
| assert prompt == agent_doc | ||
|
|
||
| def test_blank_docstring_uses_inherited_doc(self): |
There was a problem hiding this comment.
This test is failing, but it's also just a very weak and brittle test and should be deleted entirely. Not all tests are created equal.
There was a problem hiding this comment.
Good to merge once this is addressed.
This PR attempts to partially address #567 for agent:
Some subtleties that might need adjustment: