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

Basic Visualisation #15

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

Basic Visualisation #15

wants to merge 69 commits into from

Conversation

danpalmer
Copy link
Contributor

@danpalmer danpalmer commented Dec 27, 2017

This PR introduces visualisation of state machines and labels locations in them.

Note: this has been handy for debugging configs but low priority and will likely not be finished and merged before launch.

image

TODO:

  • explore linters
  • tidy up the JS (remove inline onchange, etc.)

@PeterJCLaw
Copy link
Contributor

@danpalmer were there any particular things you thought that the example visualisation should do? The example.yaml contains forks and merges in the graph, as well as both gates and actions, so I'm proposing that we build the example visualisation around that.

@danpalmer
Copy link
Contributor Author

The main things I wanted to do were around readability. I remember it being pretty basic and not very clear. Destinations, triggers, defaults, etc would be good to get on there, maybe the field used to control the destination selection, etc.

@PeterJCLaw
Copy link
Contributor

@danpalmer since we discovered that pydot does indeed depend on having graphviz installed, thoughts on letting the render be in JavaScript? https://www.graphdracula.net/ looks fairly reasonable (and appears to allow annotations on various things which we know we want to do at some point)

PeterJCLaw added 2 commits May 2, 2018 15:54
This avoids needing to worry about escaping the content of our JSON.
routemaster/config/visualisation.html Outdated Show resolved Hide resolved
routemaster/config/visualisation.html Outdated Show resolved Hide resolved
routemaster/config/visualisation.html Outdated Show resolved Hide resolved
routemaster/config/visualisation.html Outdated Show resolved Hide resolved
routemaster/config/visualisation.html Outdated Show resolved Hide resolved
routemaster/server/tests/test_endpoints.py Show resolved Hide resolved
from routemaster.config import Action, StateMachine


def nodes_for_cytoscape(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how much I like the naming being specific to the library we're using on the front end.

On the one hand, this looks like a specific format that wouldn't easily be used in other tools, but on the other, it feels like we shouldn't be encoding JS dependencies in Python naming like this.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I originally had the massively more vague name convert_to_network and change to this since the output format is specific to cytoscape. I'm not sure whether I like that the name is coupled to the library, however since the data format is pretty much coupled anyway, I think having the name be clear is better than the alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The data format could be used by other code paths and funged into whatever format is necessary.

How about state_machine_for_render?

I'd imagine we might add a label_for_render in the future too?

routemaster/state_machine/tests/test_visualisation.py Outdated Show resolved Hide resolved
routemaster/config/visualisation.html Outdated Show resolved Hide resolved
routemaster/state_machine/visualisation.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants