-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Add RecallObservations for retrieval of prompt extensions #6909
Conversation
This looks clean so far. I'm a fan of moving what we can to the event stream: much better visibility than modifying messages in-place, and I think leaning into the pub-sub approach is a good way to add extra functionality without having to tip-toe around the agent/controller/system control flow. |
I like this idea better than what I did in #6526! Happy to close that PR in favor of this one |
a103532
to
50519e7
Compare
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.
🔥 🔥 🔥
This is a really cool change, I'm excited to get some extra visibility in the event stream.
Well done!
@xingyaoww I think I addressed everything, I'd love to know what you think. There are also more tests that I separated out, that I keep an eye on, and they went well! ( I'd love to get this in, and then Calvin and I are looking into maybe doing something similar for the condenser summaries, or at least, it has become clear to us that it is a similar problem: just like |
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.
LGTM! Thanks so much for this! (Though I kinda still like the more generalized "RecallAction" better -- because environment info fits a bit awkward in "MicroagentObservation" and can be confusing to display in the UI -- I would consider it is more like "PromptExtensionAction/Observation". But happy to go ahead if you think this is good!
isinstance(obs, MicroagentObservation) | ||
and self.agent_config.enable_prompt_extensions | ||
): | ||
if obs.info_type == MicroagentInfoType.ENVIRONMENT: |
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 think we might eventually name this to other things -- MicroAgentInfo = ENVIRONMENT seems a bit confusing. But let's leave this as is for now :)
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'll give it more thought 🤔
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.
Regardless whether in this PR or not, we will probably want to make some change soon: because this PR doesn't include the system prompt, but you would like to do something similar with it, right?
So we still need Recall.
I think the system prompt just became more important to show to users, with these ContextWindowExceeded right at the start on OpenAI API: ultimately, users able to see and maybe edit it themselves would be good IMHO.
It is a bit strange! I almost stopped at half in the renaming process to think it over, because the LLM interpreted the MicroagentObservation as "the output of a MicroagentAction", which sounds like the microagent acted... That's the same confusion that existed because of the old microagents when people expected the new ones to "act" like the old ones. These events, IMHO, are about information retrieval: we found the content of a microagent or more, and published it in the stream. Or runtime info, yes! Microagents can be interpreted as a particular format in which a RecallObservation can find/bring information. Or, as you said, we could look at the end result (we do this so that eventually they're added to the context), so PromptExtensions. On the other hand, I can see maybe arguments for it:
(The 'environment' type is an enum value, maybe the UI can pick it up and do something a bit special about that type...) Idk, the near future does seem to bring more about microagents (e.g. we can maybe handle Cc: @rbren |
I trust your judgement here :) |
* Revert "move out new tests" This reverts commit cb3edc0 * move two tests * adapt to MicroagentKnowledge dataclass * adapt deduplication tests for reversed logic * move tests of the new flow from old prompt manager * adapt tests to the latest changes * rename recall -> microagent * add test for first user message * adapt to serialization change
…owledge when empty
Make microagents available in the event stream as recall observations (accessible from the UI, LLM)
This PR proposes to refactor prompt extensions into recalled observations:
Cc: @xingyaoww
I'd love your opinion about this idea. It's basically an alternative to 6526, I was curious to see roughly how it looks like, so we can maybe see if it makes sense.
Cc: @csmith49
MAIN TASKS
on_event
, initializationconversation_memory
- move context/prompt manager; handle RecallObs, event->messageRecallObs
should be before theuser message
in the messages to the LLM?Other expected outcomes:
Runtime pluginsLink of any specific issues this addresses
Fix #6535
Fix #7265
Probably fix #6191
Also fix: