Page MenuHomePhabricator

Rendering of \oinit very dense
Closed, ResolvedPublic

Description

In https://en.wikipedia.org/w/index.php?title=Wikipedia_talk:WikiProject_Mathematics&oldid=864809100 its been reported that the svg rendering of \oinit is very thick when compared to other integral

this contrasts with client side svg from normal mathjax

where it renders fine.

Event Timeline

"very thick" rather than "very think" was intended. The other issue is that the subscript C is far to the right of where it should be, as seen by looking at the other integral sign.

The other typo is that it should say \oint rather than \oinit.

The other typo is that it should say \oint rather than \oinit.

Task descriptions and summaries are wiki-like: they can be edited.

This is probably due to Mathoid having configured \oint oddly -- it adds a huge to it (via mathchoice) according to https://github.com/mathjax/MathJax/blob/master/unpacked/extensions/TeX/mediawiki-texvc.js (though I don't know whether this extension is actually used but since @Physikerwelt wrote it, something similar is probably done).

@Physikerwelt if it's ok with you, I would make a PR to MathJax to change the macro's definition. Then @WMF/MathJax could cherry-pick that commit.

Yes. That would be nice. Maybe you can report back once you have the pr for mathjax so that @SalixAlba can check if that fixes the problem.

@Physikerwelt I'll do that. But it's a "non fix" -- the macro should just be dropped from the mediawiki-texvc extension (it overwrites the already existing macro).

Here's a comparison on the client https://codepen.io/pkra/pen/wQVjbJ

great. I hope the command was already supported in the wmf mathjax version. But let's try that.

TheDJ updated the task description. (Show Details)

Cool. If you cherry-pick the change and make a PR. please also update the version npm version to ensure that the change will be included in the deployment repo and picked up for the next deployment.

Change 606732 had a related patch set uploaded (by Physikerwelt; owner: Physikerwelt):
[mediawiki/services/mathjax@master] Bump version to 2.7.2

https://gerrit.wikimedia.org/r/606732

Change 606742 had a related patch set uploaded (by Physikerwelt; owner: Physikerwelt):
[mediawiki/services/mathjax-node@master] Update to mathoid-mathjax-2.7.2

https://gerrit.wikimedia.org/r/606742

Change 606760 had a related patch set uploaded (by Physikerwelt; owner: Physikerwelt):
[mediawiki/services/mathoid@master] Update to mathoid-mathjax-node 0.7.1

https://gerrit.wikimedia.org/r/606760

Physikerwelt added a subscriber: AMooney.

@AMooney this is the change I was referring to yesterday. The upstream fix is from december 2018
https://github.com/mathjax/MathJax/pull/2095/files
it is basically delete one line.

If you understand how to backport upstream changes to the WMF fork of mathjax, you can review deploy and test everything in 15 minutes (while doing other things on the side). Most of the time is waiting. If not, the time for the "knowledge transfer" needs to be added on top.

Sure thing. I am not yet familiar with backporting upstream mathjax changes, so (as @Physikerwelt said) this will take some extra time for learning. But that's knowledge I need to acquire anyway, so should be time well spent.

@BPirkle the chain of dependencies is as follows:

mathoid depends on mathjax-node which depends on mathjax.

We need to

To be able to publish the packages to npm I need to your npm account name. Can you please send it to me either in this ticket or via email?

To be able to publish the packages to npm I need to your npm account name. Can you please send it to me either in this ticket or via email?

"bpirkle". Also added to https://phabricator.wikimedia.org/T259001

Not very creative, I know. :-)

I added you to the respective repositories.

We need to

That makes sense logically, but this is the first I've worked with these repositories. To be sure, the steps are:

  1. +2s on the first two gerrit links (mathjax and mathoid-mathjax, in that order)
  2. publish the new version of mathoid-mathjax via the npm cli
  3. +2 on the third gerrit link (mathjax-node)
  4. publish the new version of mathjax-node via the npm cli
  5. +2 on the final gerrit link (mathoid)

Is that correct? Are there additional deployment steps? Should mathoid be published on npm as well?

Is that correct?

Yes.

Are there additional deployment steps?

I do not hope so. @mobrovac did we miss something?

Should mathoid be published on npm as well?

Yes, that would be awesome.

Change 606732 merged by BPirkle:
[mediawiki/services/mathjax@master] Bump version to 2.7.2

https://gerrit.wikimedia.org/r/606732

Change 606742 merged by BPirkle:
[mediawiki/services/mathjax-node@master] Update to mathoid-mathjax-2.7.2

https://gerrit.wikimedia.org/r/606742

Change 606760 merged by jenkins-bot:
[mediawiki/services/mathoid@master] Update to mathoid-mathjax-node 0.7.1

https://gerrit.wikimedia.org/r/606760

All steps completed. How do we confirm it works as intended?

Two deployment notes, by the way:

  1. because CI isn't automatically run on some of these repos, it was necessary to first manually apply a "Verified +2" in addition to "Code Review +2", and then click Submit.
  2. I encountered a timeout after the mathoid change was merged, during the build step. It was necessary to log into Jenkins directly and click the Rerun button. (The error message had a direct link to the Jenkins page for the failed build.) The second attempt worked successfully.

All steps completed. How do we confirm it works as intended?

@BPirkle Thank you. You can create a test rendering using the rest api

https://en.wikipedia.org/api/rest_v1/#/Math/get_media_math_render__format___hash_

unfortunately it does not yet look good

https://en.wikipedia.org/api/rest_v1/media/math/render/svg/374cf26599ef917bb8983e85b24206f299649600

thus it does not seem quite done

Thanks @Physikerwelt, Will there need to be more coding done for this and then you would reach out to us when code review is needed again?

From the latest master I got the following image.


This there must be some deployment magic that did not work out so that RESTbase uses an old version of mathoid.
@hashar do you know who might know the details about which version of mathoid is actually used by restbase?

How to reproduce:

cd /tmp
git clone git@github.com:wikimedia/mathoid.git
npm i
npm test
npm start

go to localhost:10044 and enter \int \text{vs.} \oint in the q input field in the info.html page that you are automatically redirected to. Click submit.

PS: There is most likely no caching issue https://en.wikipedia.org/api/rest_v1/media/math/render/svg/858e808a365e6284c0c0526885471fd217f4328d

@BPirkle after a closer look at the logs I see that a post-merge job failed https://integration.wikimedia.org/ci/blue/organizations/jenkins/service-pipeline-test-and-publish/detail/service-pipeline-test-and-publish/1399/pipeline/ it says

+ KUBECONFIG=/etc/kubernetes/ci-staging.config

+ helm --tiller-namespace=ci install --namespace=ci --values .pipeline/values.yaml.to6pq4rw -n mediawiki-services-mathoid-aovsg0pa --debug --wait --timeout 120 https://releases.wikimedia.org/charts/mathoid-0.0.19.tgz

[debug] Created tunnel using local port: '40977'


[debug] SERVER: "127.0.0.1:40977"


[debug] Original chart version: ""

[debug] Fetched https://releases.wikimedia.org/charts/mathoid-0.0.19.tgz to /var/lib/jenkins-slave/.helm/cache/archive/mathoid-0.0.19.tgz


[debug] CHART PATH: /var/lib/jenkins-slave/.helm/cache/archive/mathoid-0.0.19.tgz


Error: release mediawiki-services-mathoid-aovsg0pa failed: timed out waiting for the condition

script returned exit code

I speculate that this is related to T261346. But the documentation on https://wikitech.wikimedia.org/wiki/PipelineLib focusses on testing rather than deployment primarily.
When trying to access https://releases.wikimedia.org/charts/mathoid-0.0.19.tgz I get a page not found. So I don't know at the moment how I could learn how this works.

@Physikerwelt Hi, I believe the problem is related to the pipelinelib issue as you mentioned. What happened is that the helm chart registry changed locations, so the deployment test failed since it's still trying to get the chart from the old location. T261346 attempts to fix this by adding the registry location to PipelineLib, but we still need to update the CI configuration to reflect those changes, which is why I created https://gerrit.wikimedia.org/r/c/mediawiki/services/mathoid/+/623480 .

I noticed that mathoid does not have a .pipeline/config.yaml file, so it can't make full use of the features in PipelineLib. I've added T261809 to migrate mathoid to the pipeline so that you can have a more self-service way to do CI.

The post-merge failure does no longer appear. Thank you. However, the mathoid instance used by restbase is still not updated. See https://en.wikipedia.org/api/rest_v1/media/math/render/svg/a8224cf9ffa1763f88263d9f6793347dd58117ce

The post-merge failure does no longer appear. Thank you. However, the mathoid instance used by restbase is still not updated. See https://en.wikipedia.org/api/rest_v1/media/math/render/svg/a8224cf9ffa1763f88263d9f6793347dd58117ce

Indeed, it needs a deployer to do so manually. Used to be Marko but he is no longer with the WMF. With @AMooney we 'll coordinate and find someone for this.

Indeed, it needs a deployer to do so manually. Used to be Marko but he is no longer with the WMF. With @AMooney we 'll coordinate and find someone for this.

I hope it can be automated one day. Gabriel, Marko, ...

Just want to let you know we've been working on improvements towards the direction of automated deploys :)

Currently Mathoid is deployed to Kubernetes via Helm, which means once a change to Mathoid is merged, we need to build a mathoid container image, update the helm values with our new image tag, and then log onto the deployment server to do the deploy.

We already have automated image building. I have done some work on automating updating the helm values that should be working in CI soon.

Just want to let you know we've been working on improvements towards the direction of automated deploys :)

Currently Mathoid is deployed to Kubernetes via Helm, which means once a change to Mathoid is merged, we need to build a mathoid container image, update the helm values with our new image tag, and then log onto the deployment server to do the deploy.

We already have automated image building. I have done some work on automating updating the helm values that should be working in CI soon.

I really appreciate this, honestly.

However, I also understand @SalixAlba and the whole math community. This community is waiting for this fix (simple fix, aka delete one line from a custom config) for almost two years. At that time there was a mathoid deployment repo ;-D Thus I am really hoping for some kind of soon that allows making some progress. It is somehow like sailing, you can't drive too slow. If it takes too long for your fixes to land in production you are effectively disabled.

I can understand the frustration.
If you want to keep updated on the automation progress, T255835 and probably T214158 are the tasks I know of at the moment.

Meanwhile, it looks like SRE and the Core Platform Team will be coordinating on helping to get mathoid deployed more regularly.

Seeing as I have been summoned by invocation of my name, I must admit a bit of confusion with the process.

Is it possible for the fix to make it into production which a bit of manual adjustment? Or will we have to wait for the automated process to work before it goes live?

Seeing as I have been summoned by invocation of my name, I must admit a bit of confusion with the process.

Is it possible for the fix to make it into production which a bit of manual adjustment? Or will we have to wait for the automated process to work before it goes live?

No, you won't have to wait for automation. Someone will be deploying manually until then. As far as I understand it, Marko who used to do these deploys left the foundation, and so the deploys have not been happening as frequently, which is what akosiaris addressed in a previous comment.

daniel triaged this task as Medium priority.Sep 9 2020, 3:35 PM

Change 626379 had a related patch set uploaded (by Holger Knust; owner: Holger Knust):
[operations/deployment-charts@master] mathoid: Update the version number to latest for deployment

https://gerrit.wikimedia.org/r/626379

Change 626379 merged by jenkins-bot:
[operations/deployment-charts@master] mathoid: Update the version number to latest for deployment

https://gerrit.wikimedia.org/r/626379

@BPirkle thank you again for your contributions. Can you estimate the effort for T148304 which is a very simiar taks. I have already implemented the fix and added a test case. Thank you.

It seems to be working but there are some caching issue.

On https://en.wikipedia.org/wiki/User:Salix_alba/sandbox there is an old probably cached version of a formula with a fat \oint and a slightly modified one with a thin \oint.

Doing https://en.wikipedia.org/wiki/User:Salix_alba/sandbox?action=purge&mathpurge=true

does not correct the image.

Not a problem after all. You need to reload the page in the browser to force new versions of the images to be downloaded.