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 Graphviz layout engines #7505

Merged
merged 14 commits into from Oct 5, 2023
Merged

Support Graphviz layout engines #7505

merged 14 commits into from Oct 5, 2023

Conversation

snehankekre
Copy link
Collaborator

@snehankekre snehankekre commented Oct 5, 2023

Describe your changes

The Graphviz renderer used on the frontend (d3-graphviz v2.6.1) supports different layout engines: dot (default), neato, twopi, circo, fdp, osage, patchwork. Streamlit uses the default “dot” engine. When users specify any other engine, we ignore their choice. This PR makes Streamlit respect the choice of engine users provide in st.graphviz_chart.

To do so we:

  • add a new field to the proto to make GraphVizChart protobuf messages support the specification of an engine
  • modify the marshall function to set this engine in the proto message
  • set the engine in the frontend GraphVizChart component before calling .renderDot

GitHub Issue Link (if applicable)

Closes #4089.

Testing Plan

  • Migrated e2e tests to playwright, and tests rendering of the charts with all engine types with snapshots
  • Update unit test
  • Spec

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@snehankekre snehankekre marked this pull request as ready for review October 5, 2023 10:32
Copy link
Contributor

@sfc-gh-lmasuch sfc-gh-lmasuch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

e2e/specs/st_graphviz_chart.spec.js Outdated Show resolved Hide resolved
@snehankekre snehankekre merged commit 31c1492 into develop Oct 5, 2023
50 checks passed
@snehankekre snehankekre deleted the fix/4089 branch October 5, 2023 20:30
eric-skydio pushed a commit to eric-skydio/streamlit that referenced this pull request Dec 20, 2023
* Support Graphviz layout engines

* Ensure radio option and engine match in test

* Update unit test

* Update snapshots

* Update comment in proto to remove 'Dot language'

* Update offending snapshot

* Delete all chromium snapshots and regenerate

* Add missing chromium snapshots

* Reintroduce 'Dot language' comment in proto

* Call engine in jest.mock

* Use default renderer when provided only spec as string

* Test if chart renders when input is a dot string

* Delete obsolete e2e test

* Upload last remaining snapshots
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Mar 22, 2024
* Support Graphviz layout engines

* Ensure radio option and engine match in test

* Update unit test

* Update snapshots

* Update comment in proto to remove 'Dot language'

* Update offending snapshot

* Delete all chromium snapshots and regenerate

* Add missing chromium snapshots

* Reintroduce 'Dot language' comment in proto

* Call engine in jest.mock

* Use default renderer when provided only spec as string

* Test if chart renders when input is a dot string

* Delete obsolete e2e test

* Upload last remaining snapshots
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
* Support Graphviz layout engines

* Ensure radio option and engine match in test

* Update unit test

* Update snapshots

* Update comment in proto to remove 'Dot language'

* Update offending snapshot

* Delete all chromium snapshots and regenerate

* Add missing chromium snapshots

* Reintroduce 'Dot language' comment in proto

* Call engine in jest.mock

* Use default renderer when provided only spec as string

* Test if chart renders when input is a dot string

* Delete obsolete e2e test

* Upload last remaining snapshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graphviz engine selection doesn't work
3 participants