-
Notifications
You must be signed in to change notification settings - Fork 240
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
Generic interface #106
Generic interface #106
Conversation
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.
Generally it looks great - seems to check all the boxes we identified with the concepts and what not. Nicely done 👏
|
||
logger = logging.getLogger() | ||
|
||
_SESSION_STREAM: ContextVar[SessionStream] = ContextVar("mentat:session_stream") |
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.
Like I said on the call - I really like this approach to 'instancing'. I think it will make sense to do the same thing with CodeContext
eventually.
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.
Yeah we might want to do this with the entire session
instance at some point
@dataclass | ||
class StreamMessage: | ||
id: UUID | ||
channel: str |
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.
Looks like this could be 'default', 'input_request', a UUID, 'input_request:{UUID}' or 'interrupt'. It'd be nice to have these as an enum or something, but I'm not sure the best way to handle the uuids..
# response = await stream.recv(f"default:{message.id}") | ||
|
||
message = await stream.send("", channel="input_request") | ||
response = await stream.recv(f"input_request:{message.id}") |
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 could imagine async tasks in the background could use this approach to. e.g. there could be an EmbeddingProvider listening on "embed_request".
Should there be an optional expire somewhere, maybe in recv, to handle network failures and the 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.
Totally. Adding more keys/values to the StreamMessage
in the future won't break anything with how it's setup atm
channel: str | ||
source: StreamMessageSource | ||
data: Any | ||
extra: Dict[str, Any] | None |
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.
What if instead of "color" we used sth like debug level - "info", "warning", "error" - which terminal.output can interpret to colors.
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.
Really love this! I'll take a closer look once it gets caught up to main, but looks good to me so far! Left a few questions on a couple things I wasn't sure about.
|
||
|
||
class Event: | ||
def __init__(self, channel: str, message: typing.Any) -> None: |
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.
nit: Why is this typing.Any instead of Any?
mentat/code_change_display.py
Outdated
|
||
|
||
def get_previous_lines(code_change, num=2): | ||
async def print_previous_lines(code_change: CodeChange, num: int = 2): |
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 did we change these to printing functions instead of just returning the string?
logger.debug("Task has stopped") | ||
|
||
self._stop_task = asyncio.create_task(run_stop()) | ||
self._stop_task.add_done_callback(cleanup_stop) |
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.
Is there a reason we need to have a done_callback instead of just doing this at the end of cleanup_stop?
I added |
There was a git merge conflict token in the
Colors do seem to be working for inserts and system messages. |
await self._main() | ||
await self._shutdown() | ||
# NOTE: if an exception is caught here, the main process will likely still run | ||
# due to background ascynio Tasks that are still running |
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.
nit: asyncio
It looks like we lost the call to
I'm setting the same errors as @jakethekoenig. I also got some logging to the terminal when running a diff:
|
Thanks for all of the comments everyone! I'm going to close and re-open this PR in an effort to get Github to show the proper git history (this might not be a github thing but we'll quickly find out lol). |
Summary
This PR covers:
mentat/terminal
Things that will be covered in a later PR:
SessionManager
to coordinate multiple, concurrent chatsTo-do
cprint
colors received from core