Dagger.jl icon indicating copy to clipboard operation
Dagger.jl copied to clipboard

Fix write dag

Open SmalRat opened this issue 1 year ago • 4 comments

Fixes for write_dag() and show_logs() functions. More details on #512 .

SmalRat avatar Jun 16 '24 01:06 SmalRat

Awesome stuff! I'm happy to accept this as-is if you'd like, but there are also a few things we can improve if you want to tackle them:

  • Add a render_logs alternative (similar to render_logs(logs, :graphviz)) that returns a renderable GraphViz object (and equivalently, we could implement show_logs(logs, :graphviz) like you've done here for rendering directly to an IO)
  • Consider adding a save_logs function that drives Cairo to save renderings to a file, or make render_logs(path::String, ...) somehow dispatch to do this (maybe via FileIO.jl)
  • Document (under docs/src/logging-visualization.md) this newly-supported renderer, so that users can more easily discover it

Let me know if you want this merged now or if you want to try to tackle some of these, I can always add these to the TODO list :smile:

jpsamaroo avatar Jun 17 '24 21:06 jpsamaroo

  • Add a render_logs alternative (similar to render_logs(logs, :graphviz)) that returns a renderable GraphViz object (and equivalently, we could implement show_logs(logs, :graphviz) like you've done here for rendering directly to an IO)
  • Consider adding a save_logs function that drives Cairo to save renderings to a file, or make render_logs(path::String, ...) somehow dispatch to do this (maybe via FileIO.jl)
  • Document (under docs/src/logging-visualization.md) this newly-supported renderer, so that users can more easily discover it

Let me know if you want this merged now or if you want to try to tackle some of these, I can always add these to the TODO list 😄

Looks like it is not a problem to add this. I will do that soon

SmalRat avatar Jun 24 '24 09:06 SmalRat

Hi!

There is an issue with implementing these features:

  • Add a render_logs alternative (similar to render_logs(logs, :graphviz)) that returns a renderable GraphViz object (and equivalently, we could implement show_logs(logs, :graphviz) like you've done here for rendering directly to an IO)
  • Consider adding a save_logs function that drives Cairo to save renderings to a file, or make render_logs(path::String, ...) somehow dispatch to do this (maybe via FileIO.jl)

It seems that GraphVizSimpleExt, which I am working on, was intended to be used when GraphViz is not imported (with GraphVizExt for the opposite case). Am I correct in this assumption?

If that is the case, GraphVizSimpleExt can not return a renderable GraphViz object, which we want from the render_logs function and which is to be used in save_logs.

SmalRat avatar Jul 09 '24 17:07 SmalRat

Hi, I'm sorry about the long delay in my reply! You are welcome to add GraphViz as a dependency to GraphVizSimpleExt - I just didn't add it since I hadn't gotten the code really working, and it didn't currently depend on it. But it definitely seems like the right thing to do!

jpsamaroo avatar Jul 30 '24 18:07 jpsamaroo