Skip to content
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

Fix #422: Markdown formatting for chat history #444

Merged
merged 3 commits into from
Feb 11, 2024

Conversation

jeanlucthumm
Copy link
Contributor

20240114_07h42m31s_grim

@jeanlucthumm jeanlucthumm changed the title Fix #422: Markdown formatting for for chat history Fix #422: Markdown formatting for chat history Jan 14, 2024
@jeanlucthumm
Copy link
Contributor Author

@TheR1D is there any procedure I need to do for assigning a reviewer?

Copy link
Owner

@TheR1D TheR1D left a comment

Choose a reason for hiding this comment

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

Thanks @jeanlucthumm
I think we need to apply Markdown rendering only for "default" role conversations. Since we can have --code conversations, and we are using pure text ouput there, markdown will skip \n and print multiple lines of code in one line.

sgpt --repl --code c1
# <- print hello and print world using Python
# -> print('hello')
# -> print('world')
# <- exit()
sgpt --show-chat c1
...
user: print hello and print world using Python                                                                                                   
assistant: print('hello') print('world') # <---

@jeanlucthumm
Copy link
Contributor Author

jeanlucthumm commented Jan 15, 2024

Ah ok good catch. Before enabling it only for default, what are your thoughts on wrapping assistant messages with markdown code block if we are in code mode? I think that would look very nice and it makes sense because the assistant is outputting code after all.

We can do this only for show_messages() by detecting mode and whether it's an assistant message.

The main concern here would be not outputting markdown wrapper when user is expected to redirect to file. But I think the use case for that is only this:

sgpt --code --chat c1 "a python file that says hello" > hello.py
sgpt --code "a python file that says hello" > hello.py

Both of which do not use show_messages

@TheR1D
Copy link
Owner

TheR1D commented Jan 15, 2024

Ah ok good catch. Before enabling it only for default, what are your thoughts on wrapping assistant messages with markdown code block if we are in code mode? I think that would look very nice and it makes sense because the assistant is outputting code after all.

We can do this only for show_messages() by detecting mode and whether it's an assistant message.

The main concern here would be not outputting markdown wrapper when user is expected to redirect to file. But I think the use case for that is only this:

sgpt --code --chat c1 "a python file that says hello" > hello.py
sgpt --code "a python file that says hello" > hello.py

Both of which do not use show_messages

You're right, we can use markdown code block when showing --code conversations with show_messages. May be we can apply md formatting for --shell conversations as well. For instance wrap assistant responses with "inline code" using backticks ``, e.g. ls -la | sort

@jeanlucthumm
Copy link
Contributor Author

jeanlucthumm commented Jan 18, 2024

Sounds good. I'll need to change how conversations are stored for this because we don't currently store the role along side the messages, only a json list, so show-chat has no way of knowing.

My idea is to change the format from a list of messages to an object like this:

{
  "role": "...",
  "messages": [
    ...
  ],
}

This is also good for future if we ever want to store more metadata about the conversations.

Will take a look this weekend.

@jeanlucthumm
Copy link
Contributor Author

Ok, @TheR1D please take a look.

Some demos:
code
default_role
shell

@TheR1D
Copy link
Owner

TheR1D commented Jan 23, 2024

Thank you, @jeanlucthumm. I will review it during this week. I believe we may need to make some changes.

@TheR1D
Copy link
Owner

TheR1D commented Jan 28, 2024

Hey @jeanlucthumm, thank you for the PR. After reviewing the changes, I believe this feature could be implemented more concisely. We can reuse the initial_message method available in ChatHandler. Here's an example:

@classmethod
def initial_message(cls, chat_id: str) -> str:
    chat_history = cls.chat_session.get_messages(chat_id)
    return chat_history[0] if chat_history else ""

Then we can slightly change logic in show_messages.

@classmethod
def show_messages(cls, chat_id: str) -> None:
    # Prints all messages from a specified chat ID to the console.
    init_message = cls.initial_message(chat_id)

    if "use and apply markdown" in init_message.lower():
        for message in cls.chat_session.get_messages(chat_id):
            if message.startswith("assistant"):
                Console().print(Markdown(message))
            else:
                typer.secho(message, fg=cfg.get("DEFAULT_COLOR"))
            typer.echo()
        return

    for index, message in enumerate(messages):
        color = "magenta" if index % 2 == 0 else "green"
        typer.secho(message, fg=color)

I think this is more suitable solution since we are following the system prompt, and it would be compatible with custom roles. It will not wrap --code conversations into a code block, but after giving it some more thought, I don't see many benefits in rendering the code using a markdown block. I think --code will mostly be used when a user wants to redirect clean code output elsewhere. If Markdown is required, we can prompt code generation using the default ShellGPT role. Could you also include tests to make sure it works as expected?

Thank you for interest and effort.

@jeanlucthumm
Copy link
Contributor Author

Sure, let's do it this way. Will take a look this weekend!

@jeanlucthumm
Copy link
Contributor Author

jeanlucthumm commented Feb 9, 2024

@TheR1D Ok I implemented your suggested change and also added unit tests. I also updated the documentation for custom roles so users are aware of this functionality.

Also let me know if you'd like me to squash commits or anything like that.

Screenshot 2024-02-09 at 12 53 30 PM Screenshot 2024-02-09 at 12 54 46 PM

Copy link
Owner

@TheR1D TheR1D left a comment

Choose a reason for hiding this comment

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

Just one minor comment, other than that, LGTM.

sgpt/handlers/handler.py Outdated Show resolved Hide resolved
@jeanlucthumm
Copy link
Contributor Author

Ok finished!

@TheR1D TheR1D linked an issue Feb 11, 2024 that may be closed by this pull request
@TheR1D TheR1D self-assigned this Feb 11, 2024
@TheR1D TheR1D added the bug Something isn't working label Feb 11, 2024
@TheR1D TheR1D merged commit f4dc37f into TheR1D:main Feb 11, 2024
3 checks passed
@TheR1D
Copy link
Owner

TheR1D commented Feb 11, 2024

Thank you @jeanlucthumm, merged.

@jeanlucthumm jeanlucthumm deleted the mdhistory branch March 1, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chat formatting when listing chat history
2 participants