Skip to content
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

Fix write dag #531

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Fix write dag #531

wants to merge 8 commits into from

Conversation

SmalRat
Copy link

@SmalRat SmalRat commented Jun 16, 2024

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

@jpsamaroo
Copy link
Member

jpsamaroo commented Jun 17, 2024

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 😄

@SmalRat
Copy link
Author

SmalRat commented Jun 24, 2024

  • 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
Copy link
Author

SmalRat commented Jul 9, 2024

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.

@jpsamaroo
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants