Make graphviz optional #27
Conversation
|
Looks good! Let's add a minor version bump for the library? Even though this technically could be backwards incompatible I think a minor version bump is fine -- its unlikely to break existing workflows. Also I think we can do the same with networkx -- it's only used in the same function. |
|
Did both. Might be worth a helper function! |
|
First up, thanks for the PR. Otherwise we need to think about the impact to documentation and onboarding; we're going from a single pip install step to multiple. Hence my curiosity as to what prompted the PR :) |
I was thinking about trying this out for computing some statistics on some single cell data (relevant library). Made me wonder whether this could even be a dependency, so I looked this packages dependencies.
Oh, I would definitely recommend having a |
Yep -- but did you install hamilton and it didn't work? or you had a dependency conflict in your environment? or?
Yep -- I would perhaps name it |
|
Just wondered "could this be a dependency", opened an issue to check, and had this PR solicited #26 (comment)
Yeah, I would also highly recommend that. I don't think that I should be the one doing that update though – since it's about reporting author metadata and license. I could undo the version bump for now? |
🤦 sorry I missed that. Thanks for clarifying.
Fair enough 😄 .
Yeah I think removing the version bump would be useful; we haven't decided on a proper release process just yet. So let us convene internally and then let you know about when we'll merge this PR. |
To ensure a delightful user experience, this commit adds
to `extras_require` for graphviz and networkx. This will mean
that users will have to do pip install sf-hamilton[visualization]
to be able to get a DAG visualized.
Rather than doing a major version bump, we're doing a minor one;
this shouldn't break any body's core use, since they shouldn't be
visualizing the DAG in production each time.
Otherwise this updates the docs with this information.
Adjusts behavior when graphivz or networkx aren't present
Rather than causing an execute to fail, we'll instead log the
error and return early.
The message to the user should look something like:
```
ERROR:hamilton.graph: graphviz is required for visualizing the function graph. Install it with:
pip install sf-hamilton[visualization] or pip install graphviz
Traceback (most recent call last):
File "/Users/stefankrawczyk/temp/hamilton/hamilton/graph.py", line 142, in display
import graphviz
ModuleNotFoundError: No module named 'graphviz'
```
skrawcz
force-pushed
the
optional-graphviz
branch
from
4abdcfa to
006523a
Compare
|
@ivirshup I went ahead and dropped your version bump, and tweaked the logic a little. Thanks for getting this started. Otherwise @elijahbenizzy I tested the install locally via |
|
@skrawcz looks good to me! Only Q is if we want to wait to release a minor version until we get the transformer/node lifecycle changes out. But I think it's fine to release this, then do my change and release a different minor version. |
Makes graphviz an optional dependency.
Fixes #26.
Additions
Removals
Testing
Not really sure how to test this.
Surely there is a way to mock existence or non-existence of a package? Cursory googling did not reveal one.
Todos
Checklist
I'm not sure I can do all of these things:
Testing checklist
Python