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

Add config visualizations in tutorial descriptions #514

Merged
merged 39 commits into from
Apr 11, 2024

Conversation

MakisH
Copy link
Member

@MakisH MakisH commented Apr 5, 2024

I think that it would give a very nice overview if in every tutorial we had a visualization of the config. It would also help popularize the config visualizer.

In terms of location, this should either go to the "Setup", or to a new section "preCICE configuration".

This is a first proof-of-concept to discuss the idea. It is currently bug-bound from precice/config-visualizer#19

In terms of PNG/SVG, the SVG would be smaller, but more complicated to view (correctly).

@uekerman what do you think?

@MakisH MakisH self-assigned this Apr 5, 2024
@uekerman
Copy link
Member

uekerman commented Apr 6, 2024

Good idea 👍
Within preECO, we could auto-generate the picture together with the Readme.

@MakisH
Copy link
Member Author

MakisH commented Apr 6, 2024

The auto-generation is also what I would aim to, once we agree on what we want to have.

@uekerman
Copy link
Member

uekerman commented Apr 7, 2024

"Configuration" section (like you have it now) sounds good to me.

once we agree on what we want to have.

What else do we still need to define?

@fsimonis
Copy link
Member

fsimonis commented Apr 9, 2024

Easy automation: use pre-commit to render each precice-config.xml to doc/images/tutorials-<CASE>-config.png

@MakisH
Copy link
Member Author

MakisH commented Apr 9, 2024

I added now a script, some CI checks, and the generated files everywhere possible.

The flow-over-heated-plate-direct cannot be visualized at the moment, due to precice/config-visualizer#22

I also fixed the aste-turbine config, which was breaking the config-visualizer precice/config-visualizer#21

The juice of this PR is in the following files:

  • tools/visualize-configs.sh
  • tools/check-visualized-config-updates.sh
  • .pre-commit-config.yaml

The first script relies on the precice-config-visualizer and on GNU parallel. I am not sure if we want to make these required in the pre-commit. Maybe we should just make a separate GitHub Actions workflow for that (or kill the parallelism). @fsimonis, what do you think?

@MakisH
Copy link
Member Author

MakisH commented Apr 10, 2024

It actually looks like the images are not even reproducible between successive runs.

@MakisH
Copy link
Member Author

MakisH commented Apr 10, 2024

I now switched to DOT for the comparisons, as these are reproducible. So, we now check:

  • Yes: If the preCICE config has changed but the DOT file has not been updated, which would imply that the SVG file has not been updated either.
  • Not: If the SVG files have been updated.

@uekerman any objections to that?

An alternative workaround would be to generate the files directly in the website CI, but then they will not be rendered in the README files of this repository (on GitHub or locally) and they will not be part of the versioning.

A final question would be if we have any reasons to avoid SVG.

A heads-up for the diff: we have 31 tutorial directories, of which one (flow-over-heated-plate-direct) cannot be visualized at the moment. The 96 files changed are:

  • 31 modified README files
  • 30 added DOT files
  • 30 added SVG files
  • 2 added shell scripts
  • 1 added workflow
  • 1 fixed config file (aste-turbine)
  • 1 modified workflow (typo fix)

@MakisH MakisH requested review from uekerman and fsimonis April 10, 2024 06:33
@MakisH MakisH changed the title Add config visualizations in tutorial setup sections Add config visualizations in tutorial descriptions and CI Apr 10, 2024
@fsimonis
Copy link
Member

fsimonis commented Apr 10, 2024

@fsimonis any idea how I could guide the config-visualizer to give me reproducible results?

Graphviz layouting uses heuristics or spring-based models to find a decent layout. So they are not reproducible. The nodes in the dot layout often look very similar, but the edges don't.

I think you are overthinking this feature a bit. Solving this with an all-in-one script is headache-inducing. pre-commit detects changes in the input XML for you.

My suggestion to save you time and pain:

  • use pre-commit to run a script for changed precice-config.xmls
  • in the script you get a list of changed configuration files
  • extract the necessary paths, then generate a visualized version of the file
  • skip if config-visualizer or dot aren't available

This way, the file is generated on the user system, which is way more likely to have the config-visualizer including dependencies installed.

@MakisH
Copy link
Member Author

MakisH commented Apr 10, 2024

This way, the file is generated on the user system, which is way more likely to have the config-visualizer including dependencies installed.

But we still need to make precice-config-visualizer an easy dependency, if we want to include it in the pre-commit

@MakisH
Copy link
Member Author

MakisH commented Apr 10, 2024

I now added also partitioned-heat-conduction-direct as a special case and switched back to PNG. I have checked the diff, and all README files seem to be render correctly and point to the right image.

Even though I am a bit unhappy that this adds generated artifacts (dot files) into the repository, I don't want to invest further work into it at the moment. We could merge this now and improve later. @fsimonis @uekerman ok?

skip if config-visualizer or dot aren't available

Not sure how I can do that. At the same time, I don't want that every change in the XML config requires people to regenerate the PNG files (which would look different for everyone). Therefore, I think the intermediate DOT files are something we can live with.

@fsimonis
Copy link
Member

I now switched to DOT for the comparisons, as these are reproducible.

This requires to pin the version of the config-visualizer

I don't want to invest further work into it at the moment. We could merge this now and improve later.

Discussion fatigue and the sunken cost fallacy are no arguments for merging a PR. This exact situation has repeatedly bitten us (especially me) in the precice repo.

The dot files are pretty significant clutter and will also be picked-up by the importer of the website, so this requires more follow-up changes.

Reproducible outputs

The dot layouter of graphviz 10.0.1 seems to generate reproducible results for both png and svg.

$ precice-config-visualizer many-participants.xml > out.dot
$ mkdir out
$ seq 1000 | parallel 'dot out.dot -Tpng > out/{}'
$ md5sum out/* | cut -f1 -d' ' | uniq
3667455412d24a04541405c52f58b841
$ dot -V
dot - graphviz version 10.0.1 (0)

I used this very complicated OpenDiHu setup to make layout shifts a likely as possible.

Conservative suggestion

We use a cron-job to auto regenerate all config visualizations in the tutorials.
The CI doesn't check if configs need to be regenerated (aka leave the contributor in peace).
We install the latest graphviz as discussed here precice/config-visualizer#22 (reproducibility + direct-access crash) and use a fixed version of the config-visualizer.

Most of the work for this has already been done here.

Radical suggestion

We render all configs consistently in the pipeline of the website.

  • we remove dot files and pngs from the tutorials repo
  • we leave the links in the Readme.md, which then show the alt text when browsing the repo
  • we generate all the config visualizations in the website pipeline with

Needs some work in the website repo.

@MakisH
Copy link
Member Author

MakisH commented Apr 11, 2024

I removed everything related to checking and CI, as well as the dot files.
I cannot invest more time for the checking part right now, feel free to continue in a separate PR.

(I just have the feeling we are running in circles and finding more bugs in dependencies, that need more time to fix, that delay everything further, for something that was supposed to be a small painless and very useful add-on since the beginning)

Copy link
Member

@fsimonis fsimonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you open an issue for the update workflow?

After that feel free to merge.

@MakisH
Copy link
Member Author

MakisH commented Apr 11, 2024

#519

@MakisH MakisH merged commit b21f4b5 into precice:develop Apr 11, 2024
1 check passed
@MakisH MakisH changed the title Add config visualizations in tutorial descriptions and CI Add config visualizations in tutorial descriptions Apr 11, 2024
@MakisH
Copy link
Member Author

MakisH commented Apr 11, 2024

Too fast... The latest precice-config-visualizer breaks the trick for the direct mesh access:

Visualizing the configuration in partitioned-heat-conduction-direct
"dot" with args ['-Tdot', '/tmp/tmpu0tnyzjr'] returned code: -6

stdout, stderr:
 b''
b"dot: compound.c:369: makeCompoundEdge: Assertion `bez->sflag' failed.\n"

Traceback (most recent call last):
  File "/home/gc/repos/precice/tutorials/.venv/bin/precice-config-visualizer", line 8, in <module>
    sys.exit(main())
  File "/home/gc/repos/precice/tutorials/.venv/lib/python3.10/site-packages/preciceconfigvisualizer/cli.py", line 88, in main
    args.outfile.write(g.create(format=ext))
  File "/home/gc/repos/precice/tutorials/.venv/lib/python3.10/site-packages/pydot/core.py", line 1786, in create
    process.returncode == 0

And changes all the images.

@fsimonis
Copy link
Member

Fixed in v1.1.1

@MakisH
Copy link
Member Author

MakisH commented Apr 12, 2024

All good now. The already checked-in script works, and the images stay the same on my system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: WP1.1 Configuration Tools (2024)
Development

Successfully merging this pull request may close these issues.

4 participants