AllenNeuralDynamics / aind-behavior-curriculum

Starter repository for behavior base primitives.
https://aind-behavior-curriculum.readthedocs.io
MIT License
1 stars 0 forks source link

Export requires graphviz #35

Open mochic opened 4 months ago

mochic commented 4 months ago

For some operating systems graphviz may not already be installed. It might be helpful for future users to have a note that they need that in the README as the error raised is informative but doesnt explicitly say "graphviz" anywhere in it and may confuse future users.

bruno-f-cruz commented 4 months ago

I think the core functionality should be independent of graphviz for sure (this was raised a few times during development). I do agree that this dependency is not explicitly stated tho. How would you feel about adding a new dependency tag to the project.toml file that includes erdantic and graphviz, and try to catch errors when calling methods that expect this lib to be installed? Would this suffice?

mochic commented 4 months ago

Yeah this sounds good. Exporting diagrams is an amazing feature. We can still use most functionality of the library by validating via just the base model validation. Any sort of documentation on why the export fails for this reason seems useful, raising specific errors makes it easier on end users I suppose, I just don't want to bog you down with these kinda smallish issues. Also, this issue existing also serves as documentation.

bruno-f-cruz commented 4 months ago

After taking a second look into this, I think we should probably use the python wrapper package PyGraphViz as it provides better syntax and makes versioning dependencies much easier for us. Thoughts @jwong-nd?

jwong-nd commented 4 months ago

Looks promising, I can try a build on a separate branch.