Skip to content

Generate the release cycle chart directly as SVG #1034

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

Merged
merged 20 commits into from
Feb 1, 2023

Conversation

encukou
Copy link
Member

@encukou encukou commented Jan 19, 2023

How's this for procrastination:
image

Please amend this PR if you want (and can); I might not get to procrastinate more soon.

Instead of Mermaid, use jinja2. Sphinx uses jinja for templating as well, so it might be possible to integrate this more tightly, but an include file works nicely for now.
(It's actually easy to generate this chart just with f-strings, but I don't want to set a bad example for people who shouldn't fully trust their input. Jinja has autoescaping to prevent SVG injections.)

Styling is done from the main stylesheet, and is bit more straightforward. I've adjusted the colours to be a bit friendlier to colour-blind people.

The labels on the boxes aren't the greatest, but I expect them to go away after #998 -- ideally replaced by a legend at the bottom.

There are vertical lines for all years now -- Mermaid's skipping of every other year was pretty confusing. Year labels have been shortened. This should work for another 10 years.

The caption is removed; it was redundant in our case.

Precise sizing is hard to do with SVG, but the font family, size and line height should nearly match the main text on big screens. At least in the current theme.

In the code, sorted_versions is now a list, and the dicts in it have some extra generated info.

Instead of Mermaid, use jinja2. Sphinx uses jinja for templating as well,
so it might be possible to integrate this more tightly,
but an include file works nicely for now.
(It's actually easy to generate this chart just with f-strings,
but I don't want to set a bad example for people who shouldn't fully
trust their input. Jinja has autoescaping to prevent SVG injections.)

Styling is done from the main stylesheet, and is bit more straightforward.
I've adjusted the colours to be a bit friendlier to colour-blind people.

There are vertical lines for all years now -- Mermaid's skipping of
every other year was pretty confusing. Year labels have been shortened.
This should work for another 10 years.

The caption is removed; it was redundant in our case.

Precise sizing is hard to do with SVG, but the font family, size and
line height should nearly match the main text on big screens. At least
in the current theme.

In the code, `sorted_versions` is now a list, and the dicts in it have
some extra generated info.
@encukou
Copy link
Member Author

encukou commented Jan 19, 2023

The current date line's position is now encoded in the SVG, rather than drawn by Javascript. With commits coming in every few weeks, it shouldn't matter. And if #998 gets in it'll ensure a commit every month or so.
It's possible to add cron to GitHub Actions, but I don't think it's worth it.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

IMO, while this is a lot of work and I'm sure you've done an excellent technical job, I would appreciate seeing a clear and concrete rationale for getting rid of the existing working mermaid approach and going with a handwritten SVG, given the amount of bespoke complexity it requires reviewing and maintaining ourselves indefinitely.

As to some possible reasons:

  • It seems hard to justify this with the minuscule fraction of users who browse with JS fully disabled (those numbers often include those using NoScript, that can easily selectively enable it for things they want, which the <noscript> message makes clear), and given the same information is available in multiple other human and machine-readable forms.
  • Making the SVG sharable and embeddable seems a somewhat more compelling reason, but right now it's just being injected directly into the page at build time (otherwise it couldn't be styled with the CSS), so in practice it doesn't appear any more easily sharable or embeddable than it was before.
  • There are some minor improvements to the appearance, but also a few rough edges that need polishing, and perhaps some of them could be made to the Mermaid version too.

Besides the complexity/maintenance issues mentioned above, unless and until #998 and #999 are implemented, it will only be updated ≈twice a year when the branches change to a different major status (and even with #998, it only adds a couple more updates; #999 would be required to update it monthly). I'm also not totally sure about the complexity of incorporating the information in #998, let alone #999 into it, as hand-writing SVG is not really in my wheelhouse.

That said, if we do go ahead with it, there are some things I noticed that should be fixed:

  • The SVG template should be given an extension other than SVG (or similar), or configured with the appropriate .gitattributes, as this currently makes it impossible to easily view it or the changes to it on GitHub due to it attempting and failing to render as an SVG image.
  • The "current date" line is shown behind all of release bars, making it much harder to see than the original. Could it be moved in front?
  • To me, the inset text (particularly "security") actually looks harder to read than before with the drop shadow, even with the larger font size.

@CAM-Gerlach
Copy link
Member

For the record, followup to #997

@hugovk
Copy link
Member

hugovk commented Jan 20, 2023

Benefits

Better rendering

This new one renders much better on mobile:

Before After

Plus when rotating from portrait to landscape, no need to reload the page for it to re-render:

Better performance

  • It seems hard to justify this with the minuscule fraction of users who browse with JS fully disabled (those numbers often include those using NoScript, that can easily selectively enable it for things they want, which the <noscript> message makes clear), and given the same information is available in multiple other human and machine-readable forms.

This also benefits people with JavaScript enabled.

It removes a 241 KB (compressed, 878K uncompressed) JS library resource.

This makes the page and the site load much quicker.

Currently the Mermaid JS is added to every page on the site, when we only need it for one page (I have an open PR to fix this: mgaitan/sphinxcontrib-mermaid#101).

It's also quicker because instead of:

  • loading JS
  • reading page data
  • generating SVG via JS
  • rendering SVG

we're now:

  • loading static SVG
  • rendering SVG

To get some numbers, compare Google's PageSpeed Insights (aka Lighthouse):

image image

This reduces: Time to Interactive, Blocking Time, Largest Contentful Paint

The time spent evaluating JS is down from 1,167 ms -> 228 ms

This "treemap" shows there's a lot less JS involved:

image image

Other things

  • Making the SVG sharable and embeddable seems a somewhat more compelling reason, but right now it's just being injected directly into the page at build time (otherwise it couldn't be styled with the CSS), so in practice it doesn't appear any more easily sharable or embeddable than it was before.

Sure, but no regression. Same also applies to the JSON. We can export them to a shareable location if we want (although we'd need to move some colours from CSS to the SVG for it to be standalone). And injecting into the page also removes an HTTP request.

  • There are some minor improvements to the appearance, but also a few rough edges that need polishing, and perhaps some of them could be made to the Mermaid version too.

👍

Complexity / maintenance

I'm going to be optimistic, it doesn't look too tricky? It's "just" some lines, rectangles and text boxes.

Regular updates

Besides the complexity/maintenance issues mentioned above, unless and until #998 and #999 are implemented, it will only be updated ≈twice a year when the branches change to a different major status (and even with #998, it only adds a couple more updates; #999 would be required to update it monthly). I'm also not totally sure about the complexity of incorporating the information in #998, let alone #999 into it, as hand-writing SVG is not really in my wheelhouse.

It does seem a problem that the "now" line is static and only rarely updated. But can we re-generate the SVG during each deploy? Possibly de-version from Git, similar to the RSS in the PEPs repo. This repo is deployed several times a week.

Re: #998 / #999, the Mermaid Gantt chart doesn't support additional sections in the same line. As noted in #998, that may also make the chart too cluttered, but we could try it out. If we decide to add extra charts instead, they shouldn't be too difficult, and templating can help if there's more than one.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

A few suggestions.

(And okay to retain Black formatting on this file?)

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

@hugovk 's rationale seems compelling enough to me :)

Remaining issues not addressed by individual comments:

  • The SVG template should be given an extension other than SVG (or similar), or configured with the appropriate .gitattributes, as this currently makes it impossible to easily view it or the changes to it on GitHub due to it attempting and failing to render as an SVG image.
  • The "current date" line is shown behind all of release bars, making it much harder to see than the original. Could it be moved in front?

hugovk and others added 2 commits January 22, 2023 12:11
@hugovk hugovk changed the title Generate the release cycle chart directly as SVG Infra: Generate the release cycle chart directly as SVG Jan 22, 2023
@hugovk hugovk changed the title Infra: Generate the release cycle chart directly as SVG Generate the release cycle chart directly as SVG Jan 22, 2023
@hugovk
Copy link
Member

hugovk commented Jan 22, 2023

  • The SVG template should be given an extension other than SVG (or similar), or configured with the appropriate .gitattributes, as this currently makes it impossible to easily view it or the changes to it on GitHub due to it attempting and failing to render as an SVG image.

Good idea, I renamed it to .svg.jinja per https://jinja.palletsprojects.com/en/3.0.x/templates/#template-file-extension.

(Funnily, https://stackoverflow.com/q/29590931/724176 suggests both .jinja and .jinja2: PyCharm highlights syntax for .jinja2 but not .jinja and VS Code highlights for .jinja but not .jinja2. I use PyCharm so added .jinja here)

  • The "current date" line is shown behind all of release bars, making it much harder to see than the original. Could it be moved in front?

Done!

Details image

https://cpython-devguide--1034.org.readthedocs.build/versions/

@hugovk
Copy link
Member

hugovk commented Jan 22, 2023

Added the final year label (e.g. '28) to the axis, but not an extra vertical line after it:

Details

Before

image

After

image

@CAM-Gerlach
Copy link
Member

To avoid these CI warnings on Windows:

warning: in the working copy of 'include/branches.csv', LF will be replaced by CRLF the next time Git touches it
warning: in the working copy of 'include/end-of-life.csv', LF will be replaced by CRLF the next time Git touches it
warning: in the working copy of 'include/release-cycle.svg', LF will be replaced by CRLF the next time Git touches it

You might want to set might want to set git config --global core.autocrlf input as the first step in the workflow, before checkout.

It does seem a problem that the "now" line is static and only rarely updated. But can we re-generate the SVG during each deploy? Possibly de-version from Git, similar to the RSS in the PEPs repo. This repo is deployed several times a week.

Meant to respond to this before, but yeah, I think that's the best approach, unless there's something I'm missing. AFAIK there's no actual need to actually check it in to the source other than it is built asynchronously from the rest of the docs and relied upon by them. So, now that we're only relying on Jinja (which is a Sphinx dep anyway, AFAIK) and it should be a fairly cheap operation, we could either just add the versions make target as a prerequisite for the main build targets, or add running the script to the appropriate Sphinx pre-build hook as a Sphinx extension (which would work for all types of builds, not just through make).

Other than these two issues and the fixed date comment above, LGTM!

@hugovk
Copy link
Member

hugovk commented Jan 22, 2023

You might want to set might want to set git config --global core.autocrlf input as the first step in the workflow, before checkout.

Done.

... we could either just add the versions make target as a prerequisite for the main build targets...

Also done.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks @encukou and @hugovk for all your work here (and putting up with my curmudgeon-ing)

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

This is quite neat!

@hugovk hugovk merged commit 21d6d8d into python:main Feb 1, 2023
@hugovk
Copy link
Member

hugovk commented Feb 1, 2023

Thanks for the reviews and to Petr for starting this off!

@hugovk
Copy link
Member

hugovk commented Feb 1, 2023

Deployed: https://devguide.python.org/versions/

If you see something like this:

image

Then do a hard reload (e.g. cmd-shift-r) to get load the new CSS:

image

@hugovk
Copy link
Member

hugovk commented Feb 1, 2023

Then do a hard reload (e.g. cmd-shift-r) to get load the new CSS:

Easier said than done on mobile... The file that needs refreshing is https://devguide.python.org/_static/devguide_overrides.css

https://docs.readthedocs.io/en/stable/hosting.html says Read the Docs busts the cache on the CDN when a new version is built, but looks like the max-age on that file is 86400 seconds (one day).

I don't see an easy way to force it to clear, short of renaming the file in Git, so guess we should just wait. (Or have users go into browser settings and clear the cache...)

@CAM-Gerlach
Copy link
Member

Easier said than done on mobile... The file that needs refreshing is devguide.python.org/_static/devguide_overrides.css

When testing the PEPs banner PR, press and holding on the reload button would trigger a hard reload in the browsers I tried, or at least appeared to.

I don't see an easy way to force it to clear, short of renaming the file in Git, so guess we should just wait. (Or have users go into browser settings and clear the cache...)

What static site generators typically do is embed a hash of the source asset as a dummy query string in the asset url that changes when the file is updated, avoiding caching. Lektor, for example, as an asseturl Jinja filter that does this. Not sure if Sphinx has something similar; this would have to be done in the theme, of course.

@pradyunsg , ideas?

@hugovk
Copy link
Member

hugovk commented Feb 2, 2023

Short term: good news, the cache has now expired, I see the chart properly on my phone.


Longer term:

It's good Furo includes a digest hash for its resources:

    <link rel="stylesheet" type="text/css" href="../_static/pygments.css" />
    <link rel="stylesheet" type="text/css" href="../_static/styles/furo.css?digest=91d0f0d1c444bdcb17a68e833c7a53903343c195" />
    <link rel="stylesheet" type="text/css" href="../_static/copybutton.css" />
    <link rel="stylesheet" type="text/css" href="../_static/styles/furo-extensions.css?digest=30d1aed668e5c3a91c3e3bf6a60b675221979f0e" />
    <link rel="stylesheet" type="text/css" href="../_static/devguide_overrides.css" /> 

But Sphinx doesn't for our CSS file:

html_css_files = [
    'devguide_overrides.css',
]

Or the other CSS files.

Maybe this could be improved in Sphinx? Maybe there's a bug in RTD not busting the cache properly?

@pradyunsg
Copy link
Member

pradyunsg commented Feb 2, 2023

sphinx-doc/sphinx#11133 is the Sphinx-side request to allow for this.

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.

5 participants