-
Notifications
You must be signed in to change notification settings - Fork 32
Enhance LLM readable markdown rendering #1569
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
Conversation
…placing subs in the LLM markdown output
4ae0f36 to
89e9732
Compare
Mpdreamz
left a comment
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! One nit around object pooling.
Did not go into output too much, we can update that as we continue to work closer with other teams.
| { | ||
| await DocumentationSet.Tree.Resolve(ctx); | ||
| var document = await markdown.ParseFullAsync(ctx); | ||
| await using var writer = new StringWriter(); |
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.
We have an object pool for stringwriters and stringbuilders and htmlrenderes which can drastically help reduce allocation e.g:
See:
docs-builder/src/Elastic.Markdown/Helpers/DocumentationObjectPoolProvider.cs
Lines 43 to 45 in bdfb9ca
| var stringBuilder = StringBuilderPool.Get(); | |
| using var stringWriter = StringWriterPool.Get(); | |
| stringWriter.SetStringBuilder(stringBuilder); |
For a usage example see:
| var subscription = DocumentationObjectPoolProvider.HtmlRendererPool.Get(); |
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 tried to create a LlmMarkdownRenderer object pool based on the existing HtmlRenderSubscription.
Unfortunately, I don't think it will work because I have the BuildContext as property on the LlmMarkdownRenderer.
But I guess I can still use it for StringWriter or StringBuilder.
WIP.
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.
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.
Ok. I just noticed, what I did is not enough. I need to return the object.
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.
OK. I think I got it right 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.
OK OK. Now I think it's nice. 9e5d96d
Otherwise it would serve the markdown file
7b51b3e to
b6d8419
Compare
Mpdreamz
left a comment
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.
On ultra nitpick but LGTM!
Closes #1504
Closes https://github.com/elastic/docs-eng-team/issues/161
Changes
This adds a new LlmMarkdownRenderer that extends Markdig's TextRendererBase.
Hence, we can take an existing MarkdownDocument and render it with our new custom renderer.
Notes
The markdown is accessible in local serve, on previews as well as assembler builds.
You just need to append
.mdto the current URL.Preview
https://docs-v3-preview.elastic.dev/elastic/docs-builder/pull/1569/syntax/quick-ref.md