-
Notifications
You must be signed in to change notification settings - Fork 205
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
Wheels for PyGraphviz - advice? #167
Comments
Thanks for working on this. Pygraphviz is a wrapper around the I haven't looked at the code/tests in a long time. It is quite possible that some rely on Thanks! |
Thanks very much for taking a look. Obviously it's no problem to rely on an installed graphviz for the tests, but I was hoping it wasn't too hard to stick to internal calls to the C libraries for the non-test code, so a wheel would be standalone and not rely on any graphviz applications. |
(Linux build) The tests you point to are failing in many places, but the failures are all coming from an extra space added: (Mac Build) The tests you point to are mostly coming from Any ideas on what package/library would control printing of spaces or empty strings? These don't cause errors on my mac installed from anaconda. Is there a good way to set up my machine to be able to recreate what travis is doing on my local machine? |
I was mistaken earlier when I said the only libraries/files needed were ``libcgraph |
The build isn't using Anaconda - it's using Python.org Python, and packages installed from pip. Is it possible that the build is compiling with something more recent (perhaps in the dependencies) than Anaconda is using? |
OK... first thing I notice: GraphViz v2.40 has a known bug for multigraphs that has been fixed in the latest unreleased version of Graphviz. That fix will be available in v2.42, but not v2.40. So all the tests I have running locally are with GraphViz v2.38. Can we switch to installing GraphViz 2.38? I don't think that will affect these test failures, but its a place to start. |
Ouch - that's harder than it seems, because graphviz appears to provide only the latest release on its site, and building from a git checkout was bothersome, maybe because of old build tools on manylinux. But I'll give it a shot. |
For NetworkX we gave up on building GraphViz from source in favor of using miniconda and its prebuilt graphviz binaries. Is that an easier route?
|
Making Graphviz from source worked on my Mac with git from
|
Yes, it works fine on Mac. The problem is with Manylinux, where the autoconf / automake / libtool versions are old. |
Is there a good known commit, on graphviz master, that we could use instead? |
It looks like linux from scratch has a copy of 2.38: And this may be helpful: And this: It seems like commit f54ac2c9 may correspond to the 2.38 release. |
OK - I think that's right, that commit f54ac2c9 is 2.38. I got graphviz building from the repo, with a bit of fiddling, and the tests pass now, on macOS and Linux, as long as graphviz is installed: https://travis-ci.org/matthew-brett/pygraphviz-wheels/builds/416335635 Now the easy stuff is done :) - the question is whether it is possible to use or even test pygraphviz without the graphviz binaries installed. |
Nice work! Because pygraphviz is essentially a wrapper around the libraries I don't think it can be used or tested easily without some sort of binaries present. That said, if libcgraph and libcdt are present we should be able to test it by creating a graph, adding an edge/node and then reporting the edges and nodes of that graph. I'm not sure I understand which question we need to address... my understanding is that we would need to include a version of those libraries in the wheel if we want it to work without a separate binary installation. If we want the wheel to work with a separate binary installation then we have to find that separate installation and that has caused most of the installation questions in github. |
I am pretty sure I don't understand this very well - but - the libraries will be present, inside the wheel - of course they have to be, in order for the extension symbols to be available. But I guess that isn't what you meant? I don't know pygraphviz at all - but to make the question more specific - is there any prospect of making some or all the examples work, just by calls into the extensions, and therefore to the embedded libraries? https://pygraphviz.github.io/examples.html Specifically, is it possible to make some of the examples work without any of the graphviz application binaries (like |
Sorry - to be specific - the |
The code itself works without the GraphViz application binaries like We could remove the drawing portions of those examples to make them testable. It does take away from the usefulness of the examples though. Perhaps we could leave that code as comments and then include one or two examples where those features are used more heavily (and not test those). Am I finally getting your questions? |
You are getting my questions - thanks for bearing with me. The reason I asked about using the application binaries, was the failures here: https://travis-ci.org/matthew-brett/pygraphviz-wheels/jobs/414015655 They are of form:
Tracking through that module, the
Is there any way of replacing these with library calls, do you think? |
Ahhh.... I see what you are saying. Yes, there are some functions which use The answer (as I understand it) unfortunately is that it is essentially impossible to replace those calls with library calls. GraphViz doesn't provide that kind of interaction. There is no API for direct library calls with GraphViz. So... it sounds like we have tools that will extract which libraries are used and include them with the wheel, but we don't have tools to automatically extract which command line tools we need available so they could be included in the wheel. This helps me realize that while these are convenience functions, their existence means we probably should include the command line tools in the wheel too.... and that leads me to question whether we should be doing this as we'll be essentially including all of GraphViz in the wheel... rats |
I'm sorry to be slow - but just to take an example. Let's say I want to unflatten my graph. I think you are saying there is no way to take the in-memory graph contained in an |
Yes, my understanding is that the applications binaries are primarily not using the libraries. |
A quick scan:
I couldn't immediately see where I guess for now we could build the wheel, then say: In order for this package to be fully functional, you should:
In the longer term, maybe we could ask the |
Here is a pdf (https://www.graphviz.org/pdf/libguide.pdf) that describes something similar to what you want to do: use GraphViz as a library. But maybe I should dive into this a little more to see how much work is actually involved. |
From the PDF, it looks as though at least the layout commands can be done from the library (section 3) and maybe the drawing (2.3). That's just at a quick read though. Maybe worth contacting the maintainers with the question? |
OK... I've looked into this more and I think it is possible to:
This would be a fairly major upgrade/change in the code base. The tests would change too. @jarrodmillman and @matthew-brett, this will not be able to be done in a timely manner. What do you think about building a wheel that doesn't include GraphViz and telling people they need to have the GraphViz tools on the os path? |
For now, we will release 1.5 without rewriting to use the library. Perhaps, we could rewrite before releasing 1.6. There is still some work needed to get the Windows wheels. Once that is done, I will release 1.5 (hopefully, before Monday 8/27). |
@matthew-brett, @dschult I am starting to work on making a new pygraphviz release. It would really like to see if we can get a binary wheel for this release. |
This thread is rather long. Does it makes sense to reticket it with actual status now that 3.0.0 is released? https://gitlab.com/graphviz/graphviz/-/blob/main/CHANGELOG.md#300-2022-02-26 If the main concern is security, then for things to improve, a |
I am not aware of any security issues. Could you explain what you mean? The reason we want wheels is so that installation is easier. In particular, so users don't need to install graphviz and let pygraphviz know where is located. I took a look at the 3.0.0 release notes and didn't see any mention of the library interface being stable now. Is it no longer considered experimental? If so, could you point us to where that is stated. If you have funding for this, please let us know. This is a volunteer project. |
@jarrodmillman in your comment from September there is a statement that upstream recommends using command line tools, because they believe the GraphViz is not memory safe #167 (comment) That's a security problem. |
Funding for security problems comes from different sources. If there is |
Opening an account on https://opencollective.com/ and structuring the things that need financing in projects will also help to raise the transparency and build up trust for funders. Yes it requires some work that it is not writing code, but it is at least give people a way for action. |
Ultimately, it would be a huge improvement for Maybe the situation has changed on Graphviz's end, but as noted there was no information in 3.0 release re: the graphviz library interface (it'd be great to ping the graphviz devs for an update though, in case there has been progress that wasn't captured in the release notes). Ultimately, I think it might be most productive to reiterate the above comments on the graphviz issue tracker. Pygraphviz already has code in place for using the Graphviz library interface: pygraphviz/pygraphviz/agraph.py Lines 1483 to 1524 in 17ba035
so in principle the building blocks are in place here as soon as the upstream project gives the green light for the library interface. |
I'm one of the Graphviz maintainers. Things have improved somewhat, but there's nothing special about the 3.0.0 release. Just a few minor breaking changes to the API that most people probably won't notice. Perhaps @Smattr can shed more light. He is doing the hard work of gradually improving the code in this respect. |
Sorry, late to the party here but I've just read through the history of this issue… The title does not make a lot of sense to me (it's a request for a WHL or a bug report about an existing WHL…?) but it seems conversation has diverged from the original topic anyway, so I'll try to respond to some things… First off, it's surprising to me that installing pygraphviz and then exec-ing an arbitrary version of a binary is expected to work. I was under the impression the norm was to bundle/build your binary dependencies inside your WHL. Isn't that what things like Numpy do? (disclaimer: I know very little about Python packaging; the preceding may be laughably incorrect).
Most of the Graphviz binaries are the classic unix thin-bin/flat-lib approach where almost all functionality lives in the library. If there is a Graphviz binary whose functionality is not exposed programmatically and you would like it to be, please let us know.
Differences between 2.38 and e.g. 2.42 should be minor. We've recently lost the ability to build 2.38, but a hero archaeologist could probably resurrect the ancient environment required to compile Graphviz 2.38. As always, the source remains eternal.
I think this is a quote from me, but if not I endorse it anyway. Basically YMMV. But a wise downstream greps their upstream’s bug tracker for things like “segfault” and “fuzzer”…
You mean you’re waiting on memory safety in a large ~40 year old C code base…? I realize this comment came from Jarrod, who knows more than most what we’re dealing with. I just want to temper the peanut gallery’s expectations.
Let me be clear… Do not run Graphviz in a security-sensitive context. <2 mins browsing the Graphviz issue tracker will yield you an RCE. There are multiple known existing stack- and heap-corruption vulnerabilities. I say this not to throw anyone under the bus, but to appeal to the same common sense that motivates you to look both ways before crossing the road. Graphviz was not intended to be run in a security-critical environment and it is primarily written in pre-ISO C. Money won’t fix this. @abitrolly if you want to improve Graphviz security, please try fixing some of the open bugs. As with every single open source project, we have too few hands and too much work. Not sure what next steps are, but feel free to tag me in anything/everything. If I can be so bold as to speak for the Graphviz maintainers collectively, we are eager and willing to help/enable whatever pygraphviz is up to. |
My point is that security concerns should not stop Python wheels from appearing. As HeartBleed and
If you can point me to the compiled binaries, I can pack them into wheels for Linux. I've already done it in https://github.com/abitrolly/ksykaitai (although hadn't found the time to release the package). |
Thanks @Smattr !! That is helpful to know. We currently use the You state that
Does "in the library" mean there is a programmatic API to get the layouts? Or does it mean that the command line tools are provided by the library? We had an Issue on the Graphviz gitlab site about a year ago, and it has been closed. Perhaps we need to open a new one. Or perhaps we need to find a way to "build wheels" that allow for command line interaction with the contents of the wheel (do we include PowerShell in the wheel for windows os?). But it'd be better/cleaner/nicer to have Graphviz provide what is available on the command line through API calls to e.g. the cgraph library. |
Sorry I realize I typoed that. I meant thin-bin/fat-lib. Most of the functionality lives in the libraries, with the command line tools as typically a
The title of this issue is “Build GraphViz in Windows for CI” and Graphviz is already built on Windows in CI. Though this issue actually seems to be about wanting to build a subset of Graphviz. Graphviz has three parallel build systems: Autotools, MS Build, CMake. We've been trying to unify everything into CMake, but this effort has been ongoing for literally years. On Windows you can choose MS Build or CMake, though some components haven't yet been integrated into CMake. Either of these build systems should let you selectively build only the libraries you want. |
Thanks for the updates @Smattr , this is all really helpful. I'll admit that I never really pursued Graphviz's library interface because I had some nebulous idea from a github/gitlab discussion somewhere that it was a bad idea. It sounds like that impression of mine was incorrect, and that there is not inherent blocker to using library calls instead of the CLI. My impression from the above and related discussions is that there's no time like the present to try the library interface! It seems like there's a relatively straightforward (famous last words) path forward here:
IMO providing a pygraphviz wheel would be the single biggest improvement the project could make --- the vast majority of issues on the issue tracker are installation issues and I would estimate that the vast majority of those would be solved out-of-hand by providing a wheel. It sounds like Graphviz's library interface is mature enough (and probably was last year as well, but I had misinterpreted the caveats) to give this a try! |
I definitely second cibuildwheel, it should be the least painful way for sure! |
A quick update from having looked at this recently: Graphviz's library interface for the layout+rendering functions should make it possible to provide wheels with the layout functionality encapsulated. There are still some functions that don't have a library interface e.g. https://gitlab.com/graphviz/graphviz/-/issues/2194 I still think it's worth pursuing wheels at this stage even if pygraphviz won't be able to fully encapsulate all of the currently available functionality - I suspect layout+rendering covers a significant chunk of current use-cases, so providing wheels that support it is worthwhile and would benefit many users. |
Well 6 years late, but https://gitlab.com/graphviz/graphviz/-/merge_requests/3511 completes the work to expose |
Excellent - thanks for the update! |
Just adding a workaround I found (and not afraid to show my ignorance). I had the wheels error that others have seen. I noticed that the instructions for Manual Windows Download (on https://pygraphviz.github.io/documentation/stable/install.html ) specify install configs. - The recommended install code for Windows did not work directly for me. It failed, giving me an error. PS C:\> python -m pip install --use-pep517 `
--config-settings="--global-option=build_ext" `
--config-settings="--global-option=-IC:\Program Files\Graphviz\include" `
--config-settings="--global-option=-LC:\Program Files\Graphviz\lib" `
pygraphviz I modified the code and the install worked correctly, I no longer had the wheel issue when running the following code. py -m pip install --use-pep517 --config-settings="--global-option=build_ext" --config-settings="--global-option=-IC:\Program Files\Graphviz\include" --config-settings="--global-option=-LC:\Program Files\Graphviz\lib" pygraphviz I feel this is probably a syntax issue that anyone well initiated would translate without a thought, but I thought I would share here in case |
I spent some time working out how to build wheels, at least for Linux and Mac:
https://github.com/matthew-brett/pygraphviz-wheels
There are two big problems:
The tests still need the graphviz binaries installed
This is because the tests, at least, need the graphviz binaries on the path. I can imagine something that would be a lot more work, that shipped binaries in the wheel, probably renamed, to avoid clashing with possible system binaries - e.g.
pygv-dot
instead ofdot
.Does pygraphviz do anything useful without the binaries? Does it make any sense to ship the wheels without them?
The tests are failing with graphviz installed
The tests fail differently on Mac and Linux:
I guess this is because of subtle differences in the output of the commands, across versions.
The text was updated successfully, but these errors were encountered: