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

Update deployment diagram to include Dask 🧜‍♂️ #1392

Merged
merged 50 commits into from
Jul 6, 2022

Conversation

deepyaman
Copy link
Member

@deepyaman deepyaman commented Mar 30, 2022

Description

Closes #1321

Highly recommend merging #1489 first, so as to separate out Mermaid from MyST migration.

Development notes

Throwing up an initial version for feedback; still need to figure out stuff like left-aligning text, rendering a list (or can just use asterisks as before), etc. if it's a requirement to look more like the original one.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

@deepyaman
Copy link
Member Author

Ah, will need to set https://github.com/mgaitan/sphinxcontrib-mermaid#markdown-support up...

Maybe somebody can review the diagram first, agree whether or not we want to go this way, and then let's think about accommodating in the build. Sphinx issues usually make my fun time less fun, so rather just do it once it's decided. :D

@noklam
Copy link
Contributor

noklam commented Apr 7, 2022

I like this new diagram. :) I can't get the padding right, but it looks better to align the platform left.

flowchart TD
    A{Can your Kedro pipeline run on a single machine?} -- YES --> B[Consult the single-machine deployment guide];
    B --> C{Do you have Docker on your machine?};
    C -- YES --> D[Use a container-based approach];
    C -- NO --> E[Use the CLI or package mode];
    A -- NO --> F[Consult the distributed deployment guide];
    F --> G[What distributed platform are you using?\n\nCheck out the guides for:\n<li>Argo</li><li>Prefect</li><li>Kubeflow Pipelines</li><li>AWS Batch</li><li>Databricks</li><li>Dask</li></ul>]; 
    style G text-align:left
    H["Does (part of) your pipeline integrate with Amazon SageMaker?<br/><br/>Read the SageMaker integration guide"];

Loading

@deepyaman
Copy link
Member Author

it looks better to align the platform left.

Agreed! How did you get it right, though? I can't get it to render left aligned/with bullets on GitHub the way you did, even trying a bunch of StackOverflow answers. 😅

@noklam
Copy link
Contributor

noklam commented Apr 8, 2022

Ah, I thought Github would also show the raw markdown here. I don't know mermaid well so I also just did a random google/StackOverflow search to see which one works.


flowchart TD
    A{Can your Kedro pipeline run on a single machine?} -- YES --> B[Consult the single-machine deployment guide];
    B --> C{Do you have Docker on your machine?};
    C -- YES --> D[Use a container-based approach];
    C -- NO --> E[Use the CLI or package mode];
    A -- NO --> F[Consult the distributed deployment guide];
    F --> G[What distributed platform are you using?\n\nCheck out the guides for:\n<li>Argo</li><li>Prefect</li><li>Kubeflow Pipelines</li><li>AWS Batch</li><li>Databricks</li><li>Dask</li></ul>]; 
    style G text-align:left
    H["Does (part of) your pipeline integrate with Amazon SageMaker?<br/><br/>Read the SageMaker integration guide"];

@antonymilne
Copy link
Contributor

Thanks for working on this @deepyaman. Much as I hate to say it, I think we should actually prioritise getting this working in sphinx as the most important requirement. After all, the way that most users are going to interact with this diagram is through our built documentation on RTD rather than browsing our docs on Github.

I see there are plugins available which sound like they should work straight away? But I do know that nothing in sphinx is as easy as it should be 😅 https://pypi.org/project/sphinxcontrib-mermaid/

If we can get this working in sphinx then I'd be very happy to change the diagram. There's absolutely no requirement for it to closely resemble the current one, so feel free to use your imagination in terms of formatting, content, whatever. Is there any way we can link the relevant boxes of the diagram to the relevant pages in the docs? No worries if not, but would be super cool if we could.

@deepyaman
Copy link
Member Author

If we can get this working in sphinx then I'd be very happy to change the diagram.

@AntonyMilneQB Thanks, this is what I was looking for! I'm happy to put in the effort to try and get it working in Sphinx as long as we're good with the diagram; just didn't want to if e.g. using Mermaid was out of the question. I'll try and do this as soon as I get a chance.

@antonymilne
Copy link
Contributor

Using Mermaid would definitely be great if we could get it working. If not then note we've also got at a drawio diagram for the Kedro architecture (which is then just manually saved to png I think) and some puml diagrams (in the draft folder but not actually rendered in the docs anywhere). Basically ease of future maintenance is priority here rather than using some particular system. Something which we write inline in the docs (looks like Mermaid would do this) rather than needing any external files sounds best to me if we can get it working.

@noklam
Copy link
Contributor

noklam commented Apr 20, 2022

Hopefully it would work and we can have a diagram for hooks lifecycle later.

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
@deepyaman deepyaman requested a review from idanov as a code owner April 26, 2022 22:27
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
…es.md

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
…note directives

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
find docs/source/ -type f -name '*.md' -exec perl -i -p0e 's/```eval_rst\n\.\. note:: */```{note}\n/g' {} +

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
@antonymilne
Copy link
Contributor

I've spent quite a while looking at how to get the svg rendering and here's where things stand. Thanks very much to @tynandebold for helping look into this.

By default the sphinx-mermaid plugin points to the minified package https://unpkg.com/mermaid/dist/mermaid.min.js in the script tag (relevant code in the plugin that handles it). If we change this to point to the unminified version then the svg renders correctly. This is done with the following lines in conf.py:

app.add_js_file("https://unpkg.com/mermaid/dist/mermaid.js")
mermaid_version = None

It is a complete mystery why it works with the unminified version but not the minified one.

Now the problem with using the unminified version is that it adds a relatively hefty javascript package to the docs page load, and from what I've read even these days with our superfast internets this is not really encouraged. In our case:

  • minified version adds 282kB to the page load
  • unminified version adds 763kB to the page load

For comparison, the docs page without mermaid at all is about 750kB, so the unminified version roughly doubles the page load. The consequence of this is a slower page loading time (but some stuff is done asynchronously so it's not as bad as doubling page load time). Personally another 700kB sounds ok to me, but the really annoying thing here is that the mermaid package will be loaded on every single docs page, even if there's no mermaid diagram there. See mgaitan/sphinxcontrib-mermaid#80.

So our options are (in my order of preference, with 1st one being best):

  1. Use the unminified mermaid package, take the page load hit on every docs page. Optional but would be nice: add support to or even better fix Load mermaid.min.js conditionally? mgaitan/sphinxcontrib-mermaid#80 ourselves (I don't think it will be fixed any time soon by anyone else)
  2. Render the diagram as a png instead. This would make it impossible to embed links, which would be a pity, and it's just less cool than an svg...
  3. Use something other than mermaid, like puml, and hope the sphinx plugin works better

P.S. the whitespace issue is also noted on the sphinx-mermaid repo but I don't think will be fixed soon. It's a problem for both png and svg renderings. mgaitan/sphinxcontrib-mermaid#82

@deepyaman
Copy link
Member Author

@AntonyMilneQB Thanks for the detailed writeup! I'm on board with the first option.

Re implementing the conditional behavior in Mermaid, https://github.com/sphinx-doc/sphinx/pull/8631/files looks like a good start (would add some has_mermaid_diagram condition in https://github.com/mgaitan/sphinxcontrib-mermaid/blob/0.7.1/sphinxcontrib/mermaid.py#L343).

docs/conf.py Outdated
Comment on lines 524 to 534
def install_mermaid(
app: Sphinx,
pagename: str,
templatename: str,
context: Dict,
doctree: Optional[nodes.document],
) -> None:
if doctree and doctree.next_node(mermaid):
app.add_js_file("https://unpkg.com/mermaid/dist/mermaid.js")


Copy link
Contributor

@antonymilne antonymilne Jul 5, 2022

Choose a reason for hiding this comment

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

Does this add it to only pages that have {mermaid}?! 😮

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! But it still isn't working properly, and it's driving me insane...

I've also tried creating a local copy of the mermaid.js (or mermaid.min.js) file under _static, and it loads fine, but doesn't render. I think it does work if I just app.add_js_file("js/mermaid.min.js") without this conditional handler, and it adds to all the files.

I'm going to try reverting back to adding to all files, and then taking the approach of removing the script from all pages that don't have a mermaid node. My current hypothesis is that the rendering of the text that get's converted to a Mermaid diagram is messed up without this, but really not sure...

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
@deepyaman
Copy link
Member Author

deepyaman commented Jul 6, 2022

@AntonyMilneQB In the end, the solution I landed on after much trials and tribulations is very similar to sphinx-doc/sphinx#6241 (comment). With this solution, we've got the minified mermaid.js file, only on pages with Mermaid directives, and without any extraneous "None" scripts. Tested in GitPod (make build-docs && cd docs/build/html/ && python -m http.server 8000):

image

image

@antonymilne
Copy link
Contributor

Ahh amazing, congratulations!! 🎉 🥳 Thank you for persevering with this. That is the perfect solution.

Do you know why it now works with the minified js? Were the None scripts somehow interfering with that but not the unminified one?

@noklam
Copy link
Contributor

noklam commented Jul 6, 2022

Nice to see it working! When Mermaid is enabled we should definitely add a chart for the hooks.

@deepyaman
Copy link
Member Author

Ahh amazing, congratulations!! 🎉 🥳 Thank you for persevering with this. That is the perfect solution.

Do you know why it now works with the minified js? Were the None scripts somehow interfering with that but not the unminified one?

I was actually able to take your suggestion:

app.add_js_file("https://unpkg.com/mermaid/dist/mermaid.js")
mermaid_version = None

This works for me, even pointing to the minified JS. It just doesn't work with the default behavior of the plugin, which would perform the app.add_js_file independently. No idea why...

(Also no idea why I have to app.add_js_file to all files, then remove from all but one file, and just adding to the one file doesn't work. To be honest, the resulting HTML looked fine, and looks fine in a lot of these "non-working" attempts. Still suspicious that something messes up the Mermaid diagram text spacing, but I haven't been able to verify, since the text inside the <div class="mermaid"> get's replaced in the working versions.)

None script cleanup is just to reduce ugly, unnecessary errors; it doesn't functionally change anything.

@antonymilne
Copy link
Contributor

Thanks for the explanation! Some things will forever remain a mystery I suppose...

@antonymilne antonymilne requested a review from noklam July 6, 2022 11:50
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

🙏🏼

@hugovk
Copy link

hugovk commented Dec 18, 2022

Hi! Thanks for figuring this out!

I've made an upstream PR, based on this, to only install the Mermaid JavaScript where needed.

Please could you have a look at mgaitan/sphinxcontrib-mermaid#101 and test it out?

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update deployment diagram to include Dask
5 participants