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

Support for graphviz rendering in outputs #73

Merged
merged 1 commit into from
Mar 30, 2025
Merged

Support for graphviz rendering in outputs #73

merged 1 commit into from
Mar 30, 2025

Conversation

curtis-allan
Copy link
Contributor

Description

This PR changes the render_outputs() function to support graphviz diagram outputs. The main problem was the order of return statements in render_output(), which was returning the text/plain data before being able to reach the image/svg+xml output type.

Since it's an image mimetype, I've moved the svg check into the include_imgs if statement check so the svg data is returned before any plain-text output.

Comparison

Screenshot 2025-03-28 at 1 09 02 pm

@curtis-allan curtis-allan added the enhancement New feature or request label Mar 28, 2025
@curtis-allan curtis-allan self-assigned this Mar 28, 2025
Copy link

gitnotebooks bot commented Mar 28, 2025

Found 1 changed notebook. Review the changes at https://app.gitnotebooks.com/AnswerDotAI/execnb/pull/73

@curtis-allan curtis-allan marked this pull request as draft March 28, 2025 03:17
@jph00
Copy link
Contributor

jph00 commented Mar 29, 2025

This is looking good to me -- have you tested it @curtis-allan, and if so, is it ready to move out of draft now?

@curtis-allan curtis-allan marked this pull request as ready for review March 29, 2025 22:50
@curtis-allan
Copy link
Contributor Author

@jph00 Yep - Added some inline tests to the notebook, can't see any reason it should cause issues outside of these niche cases.

Force-Pushed to fix CI errors & squash the history. Seems to be working well!

@jph00
Copy link
Contributor

jph00 commented Mar 30, 2025

Thanks @curtis-allan . Those tests are too long and make it harder to read the nb. See the rest of the nb to get a sense of the style we're aiming for. Tests should be really minimal and just show the specific piece of functionality of interest. You probably don't want to be relying on LLMs to do this for you -- whilst they can help show ideas and direction, they're extremely verbose and don't care about wasting our time!

@curtis-allan
Copy link
Contributor Author

curtis-allan commented Mar 30, 2025

Thanks for the feedback @jph00, will fix it up and rebase to a single commit :)

Edit: Tests for multipart mimebundles are already included in the NB, so decided it best to remove the extra test cases. Have tested in-depth my end and all looks good, was simply a case of display_data return order differing from Jupyter's mime-type rendering priority list.

@jph00 jph00 merged commit 19a1cd5 into main Mar 30, 2025
2 checks passed
@curtis-allan curtis-allan deleted the graphviz_render branch March 30, 2025 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants