-
Notifications
You must be signed in to change notification settings - Fork 4k
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 Markdown Render Support to GPT completions #56
Add Markdown Render Support to GPT completions #56
Conversation
@praveen-palanisamy approved this pull request. It is working as expected. |
Hi @kishorerv93 : Thank you for reviewing, testing and confirming that this PR works as expected and resolves your issue. We need a review from @jongio / @chuwik / @pablocastro or another core dev with write access to approve/merge this. |
bumping this so that @jongio / @chuwik / @pablocastro can review this to get it implemented. |
@praveen-palanisamy for me it doesn't seem to work properly. |
I suspect that the actual Markdown returned by the second question included backticks, and thats why the Markdown renders as code. Here's a deployed version which includes the markdown renderer: |
@pamelafox : Resolved the merge conflicts with the latest |
@praveen-palanisamy Thanks for updating to main! I've integrated this change in this demo app here to test it out: Now that we've added streaming to the repo, this change does introduce a slight visual hiccup when it's interpreting certain incremental Markdown, and some of those can be jarring. I screen capped it happening: Glitch - turned list item into a heading: Fixed a moment after - back into a normal list item: I haven't dug into precisely the underlying Markdown causing the glitch, but I think we'd want some solution before merging into main, to avoid that jarring effect. It looks like it may be related to citations, and we might need to make sure that citations get streamed a whole citation at a time, but I'm not certain that's the case. If you have time to look into it, let us know what you find out. |
cc @SteveJSteiner who authored the streaming change (see comment above on interaction with this PR) |
That's a good observation! If we have Markdown rendering enabled while streaming (partial fragments/chunks of) output text, that's an inevitable artifact, I think. I have observed this behavior with the official ChatGPT webapp and also with the Bing (Enterprise) Chat. A sample repro below where the partial markdown bold text takes a few more bytes to be complete and render properly. This output didn't include Markdown headers to match with your above screenshot, but you can see the behavior: StreamingMarkdownRender-BingChatEnterprise.mp4If this is an unacceptable UX, one way to handle this is to add another post-processing step in |
Yes, I have seen chatGPT with some visual ticks due to re-rendering of markdown. Table as the output hits this. I can also reproduce some oddities in Bing as well as it creates annotated lists. @pamelafox - what kind of bar do you wish to apply for these cases? Adding markdown support seems highly worthwhile. Can the markdown support be disabled if a user customizing the demo repo finds it too jarring for their end customers? Turing off the streaming in those cases is also an option. |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed. |
@pamelafox : Could you please provide your final comments to finalize this PR soon? @microsoft-github-policy-service agree |
Okay, so I think what we could do is just change our CSS so that headings inside the chat boxes aren't actually very large, that way it isn't such a jarring visual glitch. How about that? |
Wanted to add the feedback that markdown support is heavily requested by our users. We have many tables in our output. |
Hi @pamelafox . Thanks for the great work guys! Do you know why this PR was not closed yet? |
I've added remark-gfm, which will render the tables, since that seems to be a desire of the folks following this PR. I've also changed our MarkdownViewer to use the same package (react-markdown), for consistency. The markdown viewer originally used react-markdown in the PR, but I asked them to change to marked for package size reasons. However, the react-markdown package size seems to have decreased, and its snyk score increased, and it has more functionality, so it seems okay to use it in both places. Some screenshots: |
I also updated the system prompts which were previously discouraging markdown. Now they say nothing, with the assumption that LLMs tend toward markdown. |
I've also asked @mattgotteiner to take a look, since this increases JS size and slightly changes the system messages. |
@@ -57,7 +57,7 @@ def __init__( | |||
def system_message_chat_conversation(self): | |||
return """Assistant helps the company employees with their healthcare plan questions, and questions about the employee handbook. Be brief in your answers. | |||
Answer ONLY with the facts listed in the list of sources below. If there isn't enough information below, say you don't know. Do not generate answers that don't use the sources below. If asking a clarifying question to the user would help, ask the question. | |||
For tabular information return it as an html table. Do not return markdown format. If the question is not in English, answer in the language used in the question. | |||
If the question is not in English, answer in the language used in the question. |
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.
Did we need to explicitly ask for markdown in the prompt?
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.
In my experience, LLMs tend to produce Markdown, but likely would produce even more if we explicitly asked for it. Perhaps I should add "If your answer includes lists, headings, tables, or emphasis, use Markdown syntax."
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 surprisingly small change, especially when you don't consider the test regenerations. Great!
@mattgotteiner For the record, I did just run evaluations on the prompt change, and there are no change in metrics. Of course, we don't have a "how markdowny is this?" metric currently, but answer quality seems unaffected. |
Thanks so much @praveen-palanisamy ! Sorry it took more than a year to merge. |
Purpose
The frontend on the
main
branch currently doesn't support rendering Markdown in the model's responses/completions. This PR adds support for rendering Markdown while retaining support for raw HTML code in the completions.Fixes #30
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
How to Test
Run backend & frontend on Azure or locally and test with a prompt that would result in GPT's response/completion containing Markdown syntax. Example prompt:
What to Check
Verify that the UI renders Markdown syntax as expected.
Other Information
react-markdown
which is safe by default (nodangerouslySetInnerHTML
or XSS attacks)