Graphviz issue 233

[chbe-helix]

GraphViz Beta Update Pull Request.

Update includes all code developed to visualize the core factor graph data structure of GraphPPL. This PR resolved issue 233.

We need feedback on the version number and implementation before this is merged. During this time we will run a few additional tests to assure compatibility with GraphPPL v4.3.3.

[albertpod]

Awesome @chbe-helix and @FraserP117! Thanks! We will start testing after IWAI!

[bvdmitri]

Amazing work, also will have more time after IWAI

[wouterwln]

I reviewed some of the code. There seems to be something wrong with the extension loading; I cannot access the structures from the extension when I load GPPL. I'll push a commit refactoring the code a bit, but let's discuss @bvdmitri and @FraserP117 how we can elegantly solve this. We should be able to come up with smth now that we're all in Oxford anyway. Dummy visualizations I was able to cook up look 1000 times nicer than what the current viz looks like though! Thanks guys!

Also on a side note, do you mind renaming the file/module to GraphPPLGraphVizExt? This way, should Julia ever change their extension functionality and expose them, it's clear that this is an extension and to what package it belongs.

Once again, thanks for the work guys!

[FraserP117]

No problem at all, let’s work on this some time while we’re all here. Yes, rename it to whatever you want, I’m easy and I think I speak for Chris and Kobus here also. Thanks!

…

On Mon, 9 Sep 2024 at 11:47 AM, Wouter Nuijten ***@***.***> wrote: I reviewed some of the code. There seems to be something wrong with the extension loading; I cannot access the structures from the extension when I load GPPL. I'll push a commit refactoring the code a bit, but let's discuss @bvdmitri <https://github.com/bvdmitri> and @FraserP117 <https://github.com/FraserP117> how we can elegantly solve this. We should be able to come up with smth now that we're all in Oxford anyway. Dummy visualizations I was able to cook up look 1000 times nicer than what the current viz looks like though! Thanks guys! Also on a side note, do you mind renaming the file/module to GraphPPLGraphVizExt? This way, should Julia ever change their extension functionality and expose them, it's clear that this is an extension and to what package it belongs. Once again, thanks for the work guys! — Reply to this email directly, view it on GitHub <#251 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKMKRHMYYRQXJ7L753FMV2TZVV4DDAVCNFSM6AAAAABNXWRDFKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZXG43TMNZVGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[wouterwln]

This type should be available when we load GraphPPL and GraphViz, I don't really see a quick fix for this but I'm sure we can find something out

[wouterwln]

BTW I really like this strategy type dispatch so I really want to keep this, great work

[bvdmitri]

I like it too, but since we cannot access them from the extension maybe just use Symbols instead, e.g. :simple_iteration and :bfs_traversal (which can be converted to their corresponding structures if necessary and yes it will be type-unstable but this is not performance critical code and the type-instability will be negligible )

[bvdmitri]

e.g. something similar to

convert_symbol_strategy_to_struct(::Val{:simple_iteration}) = SimpleIteration()
convert_symbol_strategy_to_struct(::Val{:bfs_traversal}) = BFSTraversal()

function plot(; strategy = :simple_iteration)
    strategy_struct = convert_symbol_strategy_to_struct(Val(strategy))
    ...
end

I'm think Plots use similar design too

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.47%. Comparing base (c97718a) to head (6518a5c).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/graph_engine.jl	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
- Coverage   90.61%   90.47%   -0.14%     
==========================================
  Files          15       15              
  Lines        2121     2132      +11     
==========================================
+ Hits         1922     1929       +7     
- Misses        199      203       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[chbe-helix]

I reviewed some of the code. There seems to be something wrong with the extension loading; I cannot access the structures from the extension when I load GPPL. I'll push a commit refactoring the code a bit, but let's discuss @bvdmitri and @FraserP117 how we can elegantly solve this. We should be able to come up with smth now that we're all in Oxford anyway. Dummy visualizations I was able to cook up look 1000 times nicer than what the current viz looks like though! Thanks guys!

Also on a side note, do you mind renaming the file/module to GraphPPLGraphVizExt? This way, should Julia ever change their extension functionality and expose them, it's clear that this is an extension and to what package it belongs.

Once again, thanks for the work guys!

Sure! I'll make the update now

[bvdmitri]

There seems to be something wrong with the extension loading; I cannot access the structures from the extension when I load GPPL.

Yes, because it is not possible by design of the extensions. Newly defined structures are local to the extensions and cannot be accessed from outside (without nasty hacks that can be broken in the next versions of Julia). Extensions can only add methods to the existing names using existing names but cannot define new names for the outside world.

This type should be available when we load GraphPPL and GraphViz, I don't really see a quick fix for this but I'm sure we can find something out

maybe something like plot(..., strategy = :simple) and plot(..., strategy = :bfs). Symbol can be converted to TraversalStrategy and const-propagation should make it type-stable and even if its not its shouldn't be a big issue since this code is not performance critical and most of the time will be spent in the plotting anyway.

[FraserP117]

Interesting. Thanks @bvdmitri and @wouterwln. @chbe-helix, Kobus and I will be meeting this week to discuss/address this. Apologies for the delay on my part.

I think @bvdmitri's suggestion of a "Symbol" substitution is probably best for now. I do like the type dispatching and I'd like to have it in the long run. On that, it seems - from your comments - that these types would have to be defined in base GraphPPL.jl (outside of the extension). Is that correct?

I will do some investigating now.

[FraserP117]

Greetings @bvdmitri and all,

I'd just like to ask a few clarifications and to indicate our direction. 

Thanks again @bvdmitri, for your comments regarding suggested improvements. The first of these was "Do not use include("../ext/GraphPPLGraphVizExt.jl"), but instead overload some of the methods from GraphViz e.g GraphViz.load(model) could internally call generate_dot on the underlying graph .". I use This "include" statement in the two test files that I wrote. Now I am not sure where to go from here; I don't actually use any GraphViz.jl methods; everything is constructed by means of the dot""" string macro from GraphViz.jl. Consequently, I don't think there is anything from GraphViz.jl to "overload" in the manner you describe. Perhaps this all just comes down to my ignorance regarding Julia module conventions. I'm sure there'a a way around this but I can't see one just now.

Regarding node naming, yes we will be revising the way we do this to make sure they don't get too big and unwieldy. We'll ultimately have a way to modulate their "verbosity".

I will have a look at default layouts now and perhaps just implement dot as the default instead of neato.

Once these issues have been addressed - and if there are no more issues to address - we think that this will conclude Phase 1 and we will begin implementing Phase 2 (an as, yet under-specified phase). We have several plans but we think FFG notation might be the go for Phase 2, in addition to more verbosity modulation options like node naming and plate notation.

Thanks!

[bvdmitri]

Ah, thats completely normal @FraserP117 that you aren't familiar with Julia extension system.

The problem with include("../ext/GraphPPLGraphVizExt.jl") is that it relies on internal Julia implementation of package extensions, it may change in the future versions of Julia (and it does change indeed as many people report issues in the Julia repository, but ultimately they're simply doing it wrong).

Instead of writing

include("../ext/GraphPPLGraphVizExt.jl")  # Include your module

using .GraphPPLGraphVizExt: generate_dot, show_gv, dot_string_to_pdf, SimpleIteration, BFSTraversal
# ...
gppl_model = RxInfer.getmodel(rxi_model)
# ...
gen_dot_result = generate_dot(model_graph = gppl_model; kwargs...)

The GraphPPLGraphVizExt name may or may not be available in future Julia versions and its very internal to the extension. So we should avoid using it explicitly. Instead we should overload some of the methods from the packages that we rely on. See the example below.

using GraphViz
gen_dot_result = GraphViz.load(RxInfer.getmodel(rxi_model)) # also looks much nicer and less code

In order to achieve this you should implement a new method in the GraphPPLGraphVizExt.jl file that would look something like:

import GraphViz: load

function GraphViz.load(gppl_model::GraphPPL.Model; kwargs...)
    # here is the code that uses `generate_dot` internally without need to expose it outside
    return generate_dot(model_graph = gppl_model; kwargs...)
end

This is the correct way of implementing extensions in Julia. Let me know if you have any further issues with this, but it should certainly work.

[FraserP117]

Thank you @bvdmitri.

That makes a lot more sense to me now and I see what you mean.

I have made a new branch "graphviz_issue_233_node_naming", intended to settle the initial convention for node naming. At the moment, it only hosts my attempt to overload GraphViz.load. I have re-defined/overloaded GraphViz.load inside the extension and I've attempted to use the new convention in test/visualization_tests_233_v2.jl.

I appear to be missing something - again, my ignorance with Julia is letting me down here - Julia is not recognising my overloaded version of GraphViz.load just yet. I will play around, research and see what I can do. Hopefully this will be added and the node naming will be sorted by the beginning of next week: @chbe-helix, Kobus and I are thinking that the default node naming convention should simply be {node_id}_{node_name} - taking care to treat certain node_name edge cases properly. We can dream up ways to modulate the verbosity of a node - including its actual value/s - later, perhaps in Phase 2.

Many thanks, talk soon.

Fraser

[wouterwln]

Hey guys (@FraserP117 and @chbe-helix), thanks for the fantastic work! I'd say that the PR is almost in good shape, and we can merge. I took some time to review and I think that if the hygiene of the code gets up to the quality of the base repo, we can merge the PR. Some points that have to happen before we can do that:

• In GraphPPLGraphVizExt.jl lines 687-742 there is a long comment with your original version. I think we should remove this.
• There's currently two test files, visualization_tests_233.jl and visualization tests_233_v2.jl. I want to suggest to remove either or both of these files. visualization tests_233_v2.jl also creates two .pdf files in the test/test_imgs folder, which is committed to the repository.
• I would like the tests to have the same structure and setup as the tests in the existing repository (so using @testitem instead of @testset such that we are compatible with ReTestItems

I think if these points are addressed, we can merge the PR, release a new version and your code will officially be part of RxInfer ;)

Once again, thanks guys for the amazing work!

[wouterwln]

So, this file as well as the code that creates it has to go. Ideally the tests, if they create new files, delete them immediately after testing the necessary properties. I think you can take the graph saving test as a template, I think I save a graph to disk, load it again to test some properties and then delete the file afterwards.

[FraserP117]

Great stuff @wouterwln, thanks for your comments.

I will be able to address all of your suggestions tomorrow (my 27th). I agree that a cleanup of this kind has always been necessary.

I'm looking forward to phases 2 and beyond!

[bvdmitri]

Great work everyone!