rustworkx icon indicating copy to clipboard operation
rustworkx copied to clipboard

Get list of supported image formats from `dot` instead of hardcoding

Open jlapeyre opened this issue 1 year ago • 3 comments

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.

jlapeyre avatar Mar 10 '23 05:03 jlapeyre

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).

jlapeyre avatar Mar 10 '23 18:03 jlapeyre

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).

mtreinish avatar Mar 10 '23 18:03 mtreinish

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.

jlapeyre avatar Mar 10 '23 21:03 jlapeyre