Skip to content

Commit

Permalink
MAI: call dot in tests, instead of removed function find_graphviz
Browse files Browse the repository at this point in the history
  • Loading branch information
@johnyf
johnyf committed Jul 2, 2016
1 parent 552dfd4 commit 812e3c4
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion test/pydot_unittest.py
View file
Expand Up @@ -14,7 +14,7 @@
import unittest


DOT_BINARY_PATH = pydot.find_graphviz()['dot']
DOT_BINARY_PATH = 'dot'
TEST_DIR = './'
REGRESSION_TESTS_DIR = os.path.join(TEST_DIR, 'graphs')
MY_REGRESSION_TESTS_DIR = os.path.join(TEST_DIR, 'my_tests')
Expand Down

6 comments on commit 812e3c4

@jaredhuling
Copy link

Choose a reason for hiding this comment

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

FYI this breaks a lot of packages. Is there any way to keep a deprecated version of find_graphviz()?

@johnyf
Copy link
Contributor Author

@johnyf johnyf commented on 812e3c4 Jul 13, 2016

Choose a reason for hiding this comment

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

find_graphviz is a fundamentally flawed design. All other arguments aside, it violates security, by running arbitrary files that it assumes or finds in a machine, outside the PATH. The reasons behind this change are discussed in #109, #91, #116, #2, #65.

There are two solutions to accommodate this change: add GraphViz executables to the $PATH environment variable (that is now passed to subprocess when calling dot and its siblings), or explicitly pass the path to the desired executable (necessary if you have renamed it and prefer not to create a symbolic link with the standard name) using the argument prog to the method write_* (e.g., write_pdf).

For running the tests (the above change), dot needs to be present in the $PATH, as it is on Travis CI.

If there are any examples of packages that break and cannot recover with any of these approaches, then I would be happy to learn about them. The equivalent of find_graphviz wouldn't be a solution though, because of all the problems described in the issues mentioned above. In other words, pydot should not interfere with where executables are located (and also cannot, i.e., find_graphviz is a fragile heuristic that just happens to work on some machines).

@johnyf
Copy link
Contributor Author

@johnyf johnyf commented on 812e3c4 Jul 13, 2016

Choose a reason for hiding this comment

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

Also, this is why v1.1.0 was released with no API change, and minimal other changes. It provides the same pydot that everyone has been using since 2012, available on PyPI and compatible with pyparsing. This allows pydot to progress.

If you cannot readily follow changes to pydot, please consider pinning your dependency to pydot==1.1.0, until it can be unpinned.

@nouiz
Copy link

@nouiz nouiz commented on 812e3c4 Jul 13, 2016

Choose a reason for hiding this comment

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

What break Theano is not that now only PATH is searched. The problem we have is that the function find_graphviz() disapeared. We use that function to test the user installation and provide a good user error message:

Do you think it is resonable to add that function back, but just keep it searching PATH?

https://github.com/Theano/Theano/blob/master/theano/printing.py#L41

thanks

@johnyf
Copy link
Contributor Author

@johnyf johnyf commented on 812e3c4 Jul 13, 2016

Choose a reason for hiding this comment

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

Thanks for pointing at the Theano code. That call to find_graphviz can be removed, because it was an internal function of pydot, and essentially the same error message will be raised by pydot when calling write_* or create.

The original version of find_graphviz was searching at various places, and tried hard-coded paths. This was a security issue, as is detailed in the issues mentioned above. So, that functionality was removed. This left a find_graphviz that just searches the $PATH. This functionality is available as part of the standard library in subprocess (together with much more), so find_graphviz was thus made obsolete.

Python 3.3 offers shutil.which() for this job, as discussed here. In Python 2.7, you could call subprocess and pass the environment $PATH, as done within pydot.create.

Another way to check for pydot's existence is to call create with an empty graph, as

p = pydot.Dot()
p.create()

or

p = pydot.Dot()
p.create(prog=['dot'])

p.create(prog=['dotttttt']) would raise an Exception. The output is returned as bytes, so no files left on the disk.

An equally neutral call is p.create(prog=['dot', '-?']).

To summarize, find_graphviz need not exit anymore, because it was an auxiliary function and subprocess suffices for searching the $PATH, so one should call subprocess directly. This would mean that one has to repeat the ritual that is within the create function, which is a good argument for offering such functionality in a "find_graphviz"- like function (that would use subprocess, but do a neutral call to dot). In principle, the new find_graphviz could be just a wrapper to p.create() with p = pydot.Dot(). But its equally simple for the user to call p.create() directly, and thus avoid an extra function (and so nesting, flat is better PEP 20).

A one-liner is also possible, pydot.Dot.create(pydot.Dot()), though admittedly ugly.

@nouiz
Copy link

@nouiz nouiz commented on 812e3c4 Jul 15, 2016 •
edited by johnyf

Choose a reason for hiding this comment

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

Thanks. I made the change in Theano in this PR:

Theano/Theano@aefd29d

Please sign in to comment.