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 mermaid diagrams to pipeline docs page #4837

Merged
merged 8 commits into from
May 24, 2023

Conversation

jklaise
Copy link
Contributor

@jklaise jklaise commented May 10, 2023

What this PR does / why we need it:
Add basic visualizations of example pipelines in the docs.

Special notes for your reviewer:

  • This is a draft PR meant to discuss if the visual approach is correct. After this initial review, the intention is, if possible, to factor out these diagrams in stand-alone files and include them in the relevant places in both docs and examples.
  • I've chosen to try mermaid as it's code based and easier to update and manage than external assets like images, produced with another tool. The trade-off is that the layout and styling options are more limited.
  • There are some issues with using both :caption: and :align: directives, which may be due to the docs theme

See relevant docs page here: https://seldon--4837.org.readthedocs.build/projects/seldon-core/en/4837/contents/pipelines/index.html

@ukclivecox
Copy link
Contributor

These are great - they provide an extra layer of understanding which helps

@matthewlowdon
Copy link
Member

Super! I have two minor points:

  • There are a couple of implicit output joins of type 'any'. Maybe better to show them explicitly?
  • tfsimple3's INPUT should be called INPUT1.

@jklaise
Copy link
Contributor Author

jklaise commented May 11, 2023

  • tfsimple3's INPUT should be called INPUT1.

Thanks, updated.

  • There are a couple of implicit output joins of type 'any'. Maybe better to show them explicitly?

I'm in two minds about this. In general, there are 3 types of joins that can happen in a pipeline:

  • inputsJoinType - how are inputs joined, the default is inner (all inputs need to be present) and hence omitted from the examples such as the the "Join" example
  • stepsJoin - how are outputs joined together, this is explicit (hence in these examples you have the stepsJoin: any explicitly
  • triggersJoinType - how are multiple triggers joined together, this is explicit as long as there is >1 trigger for a step.

Now, ideally I would not have done the triggersJoinType example with an explicit node as I have because it slightly abuses dataflow interpretation of edges (i.e. whilst the trigger edges going into the any node can be interpreted as a data stream, the edge going out of the any node is a bit more abstract). This is due to a limitation of mermaid as my original intention was to connect both triggers edges to the downstream node directly at the same marker and designate the marker to be any (as we've discussed internally) - but this is not possible in mermaid.

That being said, I'll put up the version with stepsJoin being explicit so we can compare what we like best.

@jklaise
Copy link
Contributor Author

jklaise commented May 11, 2023

Examples without and with explicit stepsJoin (output) step:

Model routing via tensors
Original:
image
Explicit output join:
image

Trigger joins
Original:
image

Explicit output join:
image

@cliveseldon on this last example, what happens on the output if both mul10 and add10 are triggered?

@jklaise jklaise changed the title [DRAFT] Add mermaid diagrams to pipeline docs page Add mermaid diagrams to pipeline docs page May 15, 2023
@jklaise
Copy link
Contributor Author

jklaise commented May 15, 2023

I've added explicit output join step to the two relevant examples (conditional and trigger join) as I think it adds more clarity. This PR is ready now @cliveseldon

Postponed or abandoned:

  • Sort out discrepancy between the examples in this docs page and the notebook (there isn't a 1-1 correspondence between both)
  • I looked into embedding diagrams in the example notebooks, but this is non-trivial. It would be easy to include the examples in the generated .md files, but it would require some sort of injection mechanism when executing the script that converts the .ipynb files to .md files which is also non-trivial (I would be keen to move away from notebooks in the first place as we're not using them in the docs and they're only a small convenience for executing the code, but bring a big downside when it comes to including fancy things in the docs - arguably it would be just as easy to copy-paste the code snippets from .md docs without the jupyter dependency @cliveseldon). EDIT: I suppose the notebooks are used in integration tests so perhaps not feasible to remove?

@jklaise
Copy link
Contributor Author

jklaise commented May 15, 2023

Last commit also adds a diagram for the pipeline-pipeline example:
image

@jklaise jklaise added the v2 label May 16, 2023
@ukclivecox ukclivecox merged commit 9e938bb into SeldonIO:v2 May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants