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

Get list of supported image formats from dot instead of hardcoding #830

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jlapeyre
Copy link
Collaborator

@jlapeyre jlapeyre commented Mar 10, 2023

This PR replaces a list of image formats supported by graphviz that is hardcoded in rustworkx/visualization/graphviz.py with a list that is taken dynamically from the executable dot. With the version of dot that I tested, this list had 50 entries, while the hardcoded list had 35.

The immediate reason for this PR is due to a failure in the test suite despite having a working dot on the PATH. Apparently dot can be compiled with varying support for image formats. In addition on some linux distributions, including some Ubuntu and some Fedora, the package graphviz does not support jpeg. But, installing the additional package graphviz-devel does add jpeg support. Before this PR, this would cause an untrapped error and a failure in the test suite, because only the presence of dot is checked, not which images it supports. With this PR, if your dot does not support jpeg then the test for writing a jpeg will be skipped.

This PR also implements an error message that if both pillow and graphviz are missing says both are missing. Before, an error would be thrown with one message, then after installing the dependency, an error would be thrown with the second.

Fixes #811

I won't write the following tests until it's clear this will be merged.

TODO

  • Test code paths for combinations of missing pillow, dot and support for image formats.

Potential issues

I read quite a bit of the fine documentation and forum posts trying to find how to get a list of formats. What I do here is ask for a bogus format and then parse the resulting error message, which contains a list of valid formats.

This PR replaces a list of image formats supported by graphviz that is hardcoded in
rustworkx/visualization/graphviz.py with a list that is taken dynamically from the
executable `dot`. With the version of `dot` that I tested, this list had 50 entries,
while the hardcoded list had 35.

The immediate reason for this PR is due to a failure in the test suite despite having
a working `dot` on the PATH. Apparently `dot` can be compiled with varying support
for image formats. In addition on some linux distributions, including some Ubuntu and
some Fedora, the package `graphviz` does not support jpeg. But, installing the
additional package `graphviz-devel` does add jpeg support. Before this PR, this would
cause an untrapped error and a failure in the test suite, because only the presence
of `dot` is checked, not which images it supports. With this PR, if your `dot` does
not support `jpeg` then the test for writing a `jpeg` will be skipped.

This PR also implements an error message that if both pillow and graphviz are missing
says both are missing. Before, an error would be thrown with one message, then after
installing the dependency, an error would be thrown with the second.
@jlapeyre jlapeyre marked this pull request as draft March 10, 2023 05:25
@jlapeyre
Copy link
Collaborator Author

It appears that some of the tests on macos and Windows are failing. Anyone have an idea why? I looks like an executable dot installed as part of graphviz is not found.

The tests are specified in main.yml I think. Why are test names repeated? For example python3.10-x64 macOS-latest appears twice. Once it gets a green check, and once it gets a red "x". The steps in each one are slightly different. Maybe they should have different names since they have different steps. (One of them builds and tests rustworkx).

@mtreinish
Copy link
Member

There are several classes of test jobs, here is a diagram of the current CI workflow:

Screenshot_2023-03-10_13-30-36

It looks like the rustworkx unittests and the retworkx name backwards compatibility tests (that verify the old package name works) don't have unique names so there is ome overlap between the two. I expect the retworkx variant is probably failing, although we should set things up to skip if we're missing the optional dot (I haven't looked at the test code yet so ignore me if that's not what's going on).

@jlapeyre
Copy link
Collaborator Author

Thanks for the detail.

although we should set things up to skip if we're missing the optional dot

It seems like I did something in this PR to break this, but only on some systems. Maybe graphviz is not available at all for macos ? I'll try mocking or even disabling executables locally to see if I can trigger this bug locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_graphviz / test_image_type fails
2 participants