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

Configure mathjax in sphinx config and upgrade to Mathjax v3 #33600

Closed
tobiasdiez opened this issue Mar 30, 2022 · 66 comments
Closed

Configure mathjax in sphinx config and upgrade to Mathjax v3 #33600

tobiasdiez opened this issue Mar 30, 2022 · 66 comments

Comments

@tobiasdiez
Copy link
Contributor

Configure mathjax in sphinx config, and not in a javascript file. In this way, we can get ride of the custom theme option mathjax_macros, which is not understood by other themes. This is preparation to support the furo theme, see #33601.

We also upgrade Mathjax to v3. The configs were migrated using
https://mathjax.github.io/MathJax-demos-web/convert-configuration/convert-configuration.html
with the following manual changes:

  • Remove processEscapes since this now defaults to true in v3 anyway.
  • Remove ignoreHtmlClass/processHtmlClass since they are set to the default values.
  • Remove noerrors extension, which hides error messages. Normally, there shouldn't be any errors and, if there are, its probably good to have them displayed very prominently so that reviewers can discover these issues. But I don't feel very strongly about this change. Reference: https://docs.mathjax.org/en/latest/input/tex/extensions/noerrors.html

Refs #25833

Depends on #25833

CC: @kwankyu @strogdon @haraldschilly @mkoeppe

Component: documentation

Branch/Commit: public/docs/mathjax_ext @ 89ec417

Reviewer: Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/33600

@tobiasdiez

This comment has been minimized.

@tobiasdiez

This comment has been minimized.

@tobiasdiez
Copy link
Contributor Author

comment:2

This seems to do the job for me. With the disclaimer that I could only test the tutorial and dev guide, since "reference" fails with a lot of errors on gitpod. Among them

[tensor_fr] Extension error (matplotlib.sphinxext.plot_directive):
[tensor_fr] Handler <function _copy_css_file at 0x7f47e225b4c0> for event 'build-finished' threw an exception (exception: [Errno 17] File exists: '/home/gitpod/sage-local/share/doc/sage/html/en/reference/tensor_free_modules/_static')

@tobiasdiez tobiasdiez changed the title Configure mathjax in sphinx config public/docs/mathjax_ext Configure mathjax in sphinx config and upgrade to Mathjax v3 Mar 30, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 30, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

9bc4432Migrate rest of config

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 30, 2022

Changed commit from b90316d to 9bc4432

@mkoeppe
Copy link
Member

mkoeppe commented Mar 30, 2022

comment:4

See #30296 and references within for previous efforts / discussion regarding mathjax 3

@tobiasdiez
Copy link
Contributor Author

comment:5

Thanks for the pointer. It seems the issues discussed in that ticket revolve around packaging problems. I circumvent these here by using the CDN.

@mkoeppe
Copy link
Member

mkoeppe commented Mar 30, 2022

comment:6

I would expect that people want to keep an offline version without using the CDN.

@mkoeppe
Copy link
Member

mkoeppe commented Mar 31, 2022

comment:7

Replying to @tobiasdiez:

It seems the issues discussed in that ticket revolve around packaging problems.

One of the "references within" is #25833 for the upgrade to 3.x.
It would be important to know whether #25833 comment:9 is still current, and whether it is relevant for our use of Sphinx.

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 31, 2022

comment:8

Replying to @mkoeppe:

One of the "references within" is #25833 for the upgrade to 3.x.
It would be important to know whether #25833 comment:9 is still current, and whether it is relevant for our use of Sphinx.

That feature (not compatible with latex) of mathjax2 would be relevant only in online use of mathjax, such as in jupyter notebook.

As far as I know, that "automatic line breaking" is not used in our documentation. If there is any, then that is something to be fixed.

For us, there is no reason not to upgrade to mathjax3.

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 31, 2022

comment:9

Sage cell has moved to mathjax3 (relatively) long time ago. It may be worth while to check the mathjax3 configuration used in sage cell:

https://github.com/sagemath/sagecell/blob/master/js/cell.js

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 31, 2022

comment:10

You converted single quotes to double quotes. Single quotes are conventionally used for strings containing code (not plain English). You should not change them without compelling reasons.

@tobiasdiez
Copy link
Contributor Author

comment:11

Replying to @kwankyu:

Replying to @mkoeppe:

One of the "references within" is #25833 for the upgrade to 3.x.
It would be important to know whether #25833 comment:9 is still current, and whether it is relevant for our use of Sphinx.

That feature (not compatible with latex) of mathjax2 would be relevant only in online use of mathjax, such as in jupyter notebook.

As far as I know, that "automatic line breaking" is not used in our documentation. If there is any, then that is something to be fixed.

For us, there is no reason not to upgrade to mathjax3.

Thanks for the clarification!

@tobiasdiez
Copy link
Contributor Author

comment:12

Replying to @kwankyu:

You converted single quotes to double quotes. Single quotes are conventionally used for strings containing code (not plain English). You should not change them without compelling reasons.

I wasn't aware that such a styleguide rule exists. The existing code in both files doesn't seem to follow it either - it's just a random mix of single and double quotes. I tried to make at least the lines homogeneous that I've touched.

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 31, 2022

comment:13

Replying to @tobiasdiez:

Replying to @kwankyu:

You converted single quotes to double quotes. Single quotes are conventionally used for strings containing code (not plain English). You should not change them without compelling reasons.

I wasn't aware that such a style guide rule exists.

I must say it's not a rule. But single quotes are commonly used for code strings.

The existing code in both files doesn't seem to follow it either - it's just a random mix of single and double quotes. I tried to make at least the lines homogeneous that I've touched.

Leaving single-quoted strings as they are will contribute overall homogeneity of the sage code base.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 31, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

e4e1118Convert double quotes to single quotes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 31, 2022

Changed commit from 9bc4432 to e4e1118

@tobiasdiez
Copy link
Contributor Author

comment:15

Replying to @kwankyu:

Leaving single-quoted strings as they are will contribute overall homogeneity of the sage code base.

Sadly there is not much of a homogeneity. For example, latex-macros.py contains

    args = "("
    for x in sample_args:
        count += 1
        args += str(x) + ','
    args += ')'

Anyway, I've reverted these changes.

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 31, 2022

comment:16

Replying to @tobiasdiez:

Sadly there is not much of a homogeneity. For example, latex-macros.py contains

    args = "("
    for x in sample_args:
        count += 1
        args += str(x) + ','
    args += ')'

Well.. that is embarrassing.

Anyway, I've reverted these changes.

Thank you.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 7, 2022

Changed commit from 7cc5eb7 to c92b6c0

@tobiasdiez
Copy link
Contributor Author

comment:40

Replying to @kwankyu:

Now the branch at #25833 is merged.

Presently the default is offline mathjax3, to experiment. When everything is okay, we should decide what should be the default.

Thanks for your work on this!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2022

Changed commit from c92b6c0 to 5412dcb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

46d0d69Add sage package mathjax3
6da197eRemove garbage files
bd60742Put into mathjax subdirectory
5b47d22Fix checksums
f1cdce2Merge branch 'spkg-mathjax3-trac25833'
5412dcbRename config variable to --use-cdns

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 8, 2022

comment:42

I think that offline sage documentation should be the default as it has been. So I renamed the config variable to --use-cdns. If we change the default to "online" in the future, then we can just turn on --use-cdns.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

2a186d2Fix a typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2022

Changed commit from 5412dcb to 2a186d2

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 8, 2022

comment:44

To build doc with mathjax cdn, we only need

diff --git a/src/doc/Makefile b/src/doc/Makefile
index 90bea5dfac..82134ea3b4 100644
--- a/src/doc/Makefile
+++ b/src/doc/Makefile
@@ -21,7 +21,7 @@ doc-inventory--%:
 
 # Matches doc-html--developer, doc-html--reference-manifolds etc.
 doc-html--%:
-       cd $(SAGE_ROOT) && ./sage --docbuild --no-pdf-links $(subst -,/,$(subst doc-html--,,$@)) html $(SAGE_DOCBUILD_OPTS)
+       cd $(SAGE_ROOT) && ./sage --docbuild --use-cdns --no-pdf-links $(subst -,/,$(subst doc-html--,,$@)) html $(SAGE_DOCBUILD_OPTS)
 
 # reference manual, inventory
 ifndef SAGE_ROOT

So ./configure --online-doc would do the above.

@antonio-rojas
Copy link
Contributor

comment:45

Any reason os.path.join(SAGE_SHARE, 'mathjax3') is hardcoded in docs/conf.py instead of using the MATHJAX_DIR defined in env.py?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2022

Changed commit from 2a186d2 to c42ec23

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

c42ec23Use MATHJAX_DIR

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 8, 2022

comment:47

Replying to @antonio-rojas:

Any reason os.path.join(SAGE_SHARE, 'mathjax3') is hardcoded in docs/conf.py instead of using the MATHJAX_DIR defined in env.py?

No. You are right. Thanks.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2022

Changed commit from c42ec23 to 89ec417

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

8df9661Upgrade mathjax package to mathjax3
43b802dMerge branch 'spkg-mathjax3-trac25833'
89ec417Fix MATHJAX_DIR env variable

@tobiasdiez
Copy link
Contributor Author

comment:49

Should we merge this ticket with #25833, since you replace the mathjax 2 package there, so one probably should upgrade the config in one go as well. What do you think?

@tobiasdiez
Copy link
Contributor Author

comment:50

Replying to @kwankyu:

I think that offline sage documentation should be the default as it has been. So I renamed the config variable to --use-cdns. If we change the default to "online" in the future, then we can just turn on --use-cdns.

I'm not sure what should be the default for developers (but I guess everyone has internet if they contribute to trac...so I guess cdn would make sense), but the server should use the cdn to increase the speed or not? Comparing the loading times from the cdn vs sage's server, the cdn is a factor 10 quicker (13 ms vs 140ms).

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 8, 2022

comment:51

Replying to @tobiasdiez:

Should we merge this ticket with #25833, since you replace the mathjax 2 package there, so one probably should upgrade the config in one go as well. What do you think?

I agree. Let's do it. I will upload the branch of this ticket to #25833.

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 8, 2022

comment:52

Replying to @tobiasdiez:

Replying to @kwankyu:

I think that offline sage documentation should be the default as it has been. So I renamed the config variable to --use-cdns. If we change the default to "online" in the future, then we can just turn on --use-cdns.

I'm not sure what should be the default for developers (but I guess everyone has internet if they contribute to trac...so I guess cdn would make sense), but the server should use the cdn to increase the speed or not? Comparing the loading times from the cdn vs sage's server, the cdn is a factor 10 quicker (13 ms vs 140ms).

The default is aimed at sage users with lowest resources.

I think we need to provide ./configure --online-doc option so that everyone can choose what he/she wants. Developers would of course have internet resource usually, but sometimes not (on airplane :). Personally I would prefer offline-doc.

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 9, 2022

comment:54

Replying to @kwankyu:

I think we need to provide ./configure --online-doc option so that everyone can choose what he/she wants.

No. We already have SAGE_DOCBUILD_OPTS environment variable. So we can

export SAGE_DOCBUILD_OPTS="--use-cdns"

before make.

@tobiasdiez tobiasdiez removed this from the sage-9.6 milestone Apr 9, 2022
@kwankyu
Copy link
Collaborator

kwankyu commented Apr 9, 2022

Changed author from Tobias Diez, Kwankyu Lee to none

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 9, 2022

Reviewer: Kwankyu Lee

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 9, 2022

comment:56

Replying to @tobiasdiez:

https://doc.sagemath.org/html/en/developer/trac.html#reviewing-and-closing-tickets

Okay. Let's close this.

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

No branches or pull requests

4 participants