Skip to content

bugfix: Bug in which context_id wasn't stable across REPL messages#480

Merged
eob merged 4 commits into
mainfrom
fix-history
Jul 16, 2023
Merged

bugfix: Bug in which context_id wasn't stable across REPL messages#480
eob merged 4 commits into
mainfrom
fix-history

Conversation

@eob
Copy link
Copy Markdown
Contributor

@eob eob commented Jul 14, 2023

No description provided.

@eob eob requested a review from douglas-reid July 14, 2023 06:19
douglas-reid
douglas-reid previously approved these changes Jul 14, 2023
while True:
input_text = input(colored(text="Input: ", color="blue")) # noqa: F821
output = responder(input_text)
output = responder(prompt=input_text, context_id=self.context_id)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eob I like this PR. And this seems totally fine. Do we need to be concerned at all with people trying to use the REPL for functions that aren't prompt and tripping on the context_id arg, or should we enforce that you must take context_id as an arg, or some other option.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a really good question. And I think the problem you're observing extends even to the prompt: str argument, right? There's an unstated requirement about the signature of responder.

Proposal: what if we allowed ToolRepl and AgentRepl to make these kinds of assumptions now, since that's probably the 90% case, but then we planted a stake in the ground (your enforcement suggestion above?) about the "chat I/O contract" that an AgentService must, at minimum, provide.

Here, I suppose that's implicitly:

prompt(prompt: str, context_id: str) => List[Block]

though I imagine if we were being proactive about defining it, we'd probably want to extend that a bit:

prompt(prompt: Optional[str, Block], context_id: str) => List[Block]

Or even something further..

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should standardize. Maybe with some nasty combo of lists

@eob eob enabled auto-merge (squash) July 16, 2023 01:18
@eob eob disabled auto-merge July 16, 2023 01:19
@eob eob merged commit 679061e into main Jul 16, 2023
@eob eob deleted the fix-history branch July 16, 2023 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants