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 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.57%. Comparing base (03fb54b) to head (e35e5d4).
Report is 30 commits behind head on main.

Files with missing lines	Patch %	Lines
src/graph_engine.jl	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
- Coverage   90.61%   90.57%   -0.04%     
==========================================
  Files          15       15              
  Lines        2130     2133       +3     
==========================================
+ Hits         1930     1932       +2     
- Misses        200      201       +1     

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

[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.

[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!

[FraserP117]

Hello @bvdmitri, @wouterwln and @chbe-helix,

I have recovered from COVID and I've made some changes to the extension, in addition to addressing those suggestions from @wouterwln.

First up, I have removed the show_gv function for inclusion in this phase. I was having some difficulty getting things to render in non-jupyter-notebook environments. Rather than extend the time taken to get Phase 1 finished, I have opted to remove show_gv (for now) and to rely upon the affordance of saving the visualization to a file.

I've made some basic tests - very basic - with ReTestItems. At present, I simply instantiate a few models of increasing complexity, sweep through several expected combinations of the arguments and finally test to see if the visualization was generated and saved without issue - automatically deleting all temporarily created files. I have defined several models in test/test_models.jl - most of which are taken from Kobus's LeranableLoop page. I then import specific models into the test file that I wrote: test/visualization_tests.jl.

I fixed some small issues related to the get_displayed_label functions. Their behaviour had accidentally broken the node-naming conventions that I was using. I've gone back over all the doc-strings, for all functions, and made sure they're up to date.

One issue that might remain: I have not written tests for any of the other functions in the extension. I can of course make these, if desired.

Please let me know if there are any other things to address for Phase 1. I look forward to feedback!

Many thanks!

[bvdmitri]

Nice work @FraserP117 !
Its not a big deal if internal extension functions are not fully tested as long as we have a proper integration test (which as I understand you added already so its fine imo). As a side note, I removed the ReTestItems from package dependencies, because it is already listed as a dependency in the test/Project.toml. Also I removed Revise from package dependencies because its a developer tool, users don't benefit from it. As a developer you should install Revise in your global environment, not package environment. Read more here

I think we can merge when/if tests pass

[bvdmitri]

I spent some time refining the tests and found an interesting bug that took quite a while to resolve. Occasionally, Julia can include the " symbol in structure names, which caused the rendering to be completely wrong (though still technically successful).

In the tests, I now use the model zoo from the TestUtils module, which covers more cases. Additionally, I no longer remove the files after the tests run; instead, I add them to .gitignore. This approach makes it convenient to analyze the files post-test and doesn’t clutter the repository since they aren’t committed.

[bvdmitri]

Additionally I removed RxInfer as a dependency, because ideally this functionality should (and it does) work without RxInfer.

[bvdmitri]

@wouterwln should we remove nightly from testing?

[wouterwln]

Yes we should. The fails are not our fault. We should include it later to find if base Julia breaks something again.

[bvdmitri]

We can add it after the documentation step and make it optional for the pull requests

[FraserP117]

Ahh excellent, thanks @bvdmitri,

Apologies regarding the documentation, I thought it best to err on the side of verbosity. I do like this streamlined approach however.

Yes, including ReTestItems and Revise in the package dependencies was an oversight of mine. It didn't even occur to me that this would be an issue, sorry!

I'll take a look at your modified SVG file format for saving the visualisations also. Please do reach out to ask me for anything further that you think is appropriate. I'm keen to write up documentation now - I'm not sure if you guys have a favoured way to do that for GraphPPL.jl specifically?

Talk soon!

[bvdmitri]

No worries @FraserP117 , you did a great job. Regarding documentation, well, there are different opinions of course, recently I like if documentation skips the implementation details, e.g. how it does things and explains only what does it do. It also makes it easier to change how it does things without changing the documentation because the what part won't change.

[bvdmitri]

Well done gentlemens

[FraserP117]

Thank you very much @bvdmitri! Indeed thank you all!

It's an immense honour to participate in a project that I admire so greatly.

I'm keen to move on to Phase 2 now, @chbe-helix, Kobus and I will be discussing the direction for Phase 2 later in the week. We'll be in touch with our ideas, to workshop these and to get your ultimate approval.

I must thank @chbe-helix specially for advice related to our direction, all the way down to practical Git-related stuff. My work would have suffered without his advice.

I am keen to help out with perhaps updating the section of the documentation pertaining to visualisation - and any other documentation besides. I'm always keen for critical feedback also.

I'm hungry for more and I look forward to all that is to come!

[albertpod]

@FraserP117, @chbe-helix we are honoured to have you contributing guys!

[bvdmitri]

Nice! Thanks @FraserP117 , I've started to update this section in the docs in https://github.com/ReactiveBayes/RxInfer.jl/tree/gppl-4.4.0 but we indeed need a better documentation section. Also in GraphPPL repository.

[FraserP117]

Nice! Thanks @FraserP117 , I've started to update this section in the docs in https://github.com/ReactiveBayes/RxInfer.jl/tree/gppl-4.4.0 but we indeed need a better documentation section. Also in GraphPPL repository.

Ahh nice, well I'm keen to assist with updating both repo's docs. At least that frees you up do do other things. Perhaps we can talk about it in the meeting later today.

[bvdmitri]

Yeah I will do only a basic update and set to release this version before the meeting, but it looks nice already

[FraserP117]

I found that the :bfs iteration strategy coupled with the dot layout generally made the most aesthetically pleasing and easy-to-read visualisations, especially for large models.

[bvdmitri]

I already mentioned this to @FraserP117 during our meeting, but I’d like to reiterate it here for @chbe-helix as well. Thank you both for your hard work in bringing this PR to life! This was a great start to our collaboration, and I’m looking forward to working together more in 2025 to enhance the visualization and other aspects of the RxInfer ecosystem. Wishing you a wonderful holiday season! Cheers!

[abpolym]

Nice work @FraserP117 @chbe-helix !