Page MenuHomePhabricator

Chinese conversion no longer work in the table of content
Closed, ResolvedPublicBUG REPORT

Description

List of steps to reproduce:

What happens?:
Since yesterday, multiple users have reported that Chinese conversion no longer functions in ToC on Chinese Wikipedia.

image.png (1×662 px, 179 KB)

What should have happened instead?:
ToC is converted.

Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc:
1.38.0-wmf.7 (rMW7dc8191edeb9)
2021-11-02T17:55:02

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/737150 is the quick and dirty fix, but doesn't do anything for pages which are already stored with a non-language-converted TOC in the ParserCache (which is all of them). It will fix the issue as pages are purged from ParserCache, though.

I'll try to fix this "properly" as well, but it's worth reviewing this patch and seeing if it's "good enough for the weekend".

Change 737153 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] WIP: put language conversion for ToC in the 'right' place

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

Tested 737150 locally and it seems to work. Tested 737153 as well, and it works. I think 737153 is preferable as it fixes even "old" parser cache entries.

Need to get a review & vote on which is preferable, and perhaps a cherry-pick to production if we don't want to wait for the next train.

Tested locally by setting $wgLanguageCode='crh'; in my LocalSettings.php and switching to Cyrillic; for someone illiterate in Chinese this is a much easier script to use to verify that language conversion is working.

Change 737079 had a related patch set uploaded (by Dduvall; author: C. Scott Ananian):

[mediawiki/core@wmf/1.38.0-wmf.7] Regression fix: do language conversion on ToC in ParserOutput::getText()

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

Change 737153 merged by jenkins-bot:

[mediawiki/core@master] Regression fix: do language conversion on ToC in ParserOutput::getText()

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

Change 737079 merged by jenkins-bot:

[mediawiki/core@wmf/1.38.0-wmf.7] Regression fix: do language conversion on ToC in ParserOutput::getText()

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

Mentioned in SAL (#wikimedia-operations) [2021-11-06T01:43:43Z] <dduvall@deploy1002> Synchronized php-1.38.0-wmf.7/includes/parser/ParserOutput.php: Backport: [[gerrit:737079|Regression fix: do language conversion on ToC in ParserOutput::getText() (T295187)]] (duration: 00m 56s)

Change 737150 abandoned by C. Scott Ananian:

[mediawiki/core@master] Regression fix: Ensure ToC is properly language-converted

Reason:

Abandoned in favor of Ic14b3a49a8ee7ed600485d4f8a363a206035a847

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

Legoktm lowered the priority of this task from Unbreak Now! to High.Nov 6 2021, 1:54 AM
Legoktm added a project: Wikimedia-Incident.

The issue should be fixed now, but I'm leaving this open since there's some follow-up work to be done with parser tests to prevent this kind of regression in the future. I think an incident doc would be nice to have since there seem to be things that could be learned from this, like how to make a patch like this safe to revert without requiring parser cache invalidation.

T295227 has a bit different outcome than T295187, however it's the same underlying issue due to reverting those changes which originally triggered T295003.

SD_hehua raised the priority of this task from High to Needs Triage.Nov 8 2021, 2:25 PM

Please resolve the problem on zh wikivoyage (T295227 )!

Please resolve the problem on zh wikivoyage (T295227 )!

This is unrelated. Please see my advice on T295227#7489787

Thanks all for handling this so late on Friday and +1 to writing an incident report. @cscott and I will be looking into any remaining issues today. Please ping us directly on Phab/IRC/Slack if you see any other bugs that could be related.

For future reference, if there is something that has an effect such as "ParserOutput generated in wmf6 is not the same as wmf7", please list it as a risky change in the deployment blockers task so we are aware. If we cannot safely rollback upon regression, that puts deployers in a pretty tight spot to say the least.

Unfortunately, I only realized this was risky while spending the early part of Friday heads down debugging this after realizing the LanguageConverter operated on table of contents, and alerting @cscott we'd introduced a bug. In hindsight, I think any parser change should be flagged as risky going forward given a lot of its hidden complexities regardless of whether we know that or not. I'll make sure that goes into the incident report.

Unfortunately, I did know prior to the rollback happening, so the follow-up issue of the missing table of contents could have been prevented. I wasn't expecting us to roll back the train, so I didn't raise the alarm. I'll make sure this is noted in the incident report as well. Seems like we could improve processes here.

Also for the incident report: In terms of code matters, we definitely need to cover this with some kind of test going forward and probably some tests for cached parser content. Having a project that uses LanguageConverter in group 1 wikis and having a beta cluster version enabled would have also helped flag this earlier.

Having a project that uses LanguageConverter in group 1 wikis and having a beta cluster version enabled would have also helped flag this earlier.

Actually, we do have a group 1 wiki that uses LanguageConverter in beta cluster since 2020: Chinese Wikivoyage Beta Cluster

Technically the bug should have been visible on all non-wikipedia chinese language sites, which are all in group 1. But unfortunately they don't have very high visibility.

I'm going to rephrase and repost this later today

Krinkle added a subscriber: Krinkle.

Has a parser test been written for this meanwhile? If not, is it blocked on new test primitives?

@cscott: Is this still high priority (as the original issue itself seems fixed)?

matmarex added a subscriber: matmarex.

The issue should be fixed now, but I'm leaving this open since there's some follow-up work to be done with parser tests to prevent this kind of regression in the future. I think an incident doc would be nice to have since there seem to be things that could be learned from this, like how to make a patch like this safe to revert without requiring parser cache invalidation.

It looks like this is not happening, so we should just close it, instead of leaving the zombie task open forever…

Has a parser test been written for this meanwhile? If not, is it blocked on new test primitives?

Have opened T299973 since this seems important.

It looks like this is not happening, so we should just close it, instead of leaving the zombie task open forever…

The incident report still had a few todos (mostly opening tickets), my understanding is the content transform team are still working on that: https://wikitech.wikimedia.org/wiki/Incident_documentation/2021-11-05_TOC_language_converter#Actionables cc @ssastry for prioritization. I've created a few for the ones I felt comfortable doing so for.

在T295187#7650858中,@Stang写道:

Reproduced.

Change 764112 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] ParserOutput: Use page language instead of site content language for conversion

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

Obviously not a Parsoid issue, but I understanding is Parsoid is the tag the content transform team uses so I want to make sure this is still on their radar.

Change 764112 merged by jenkins-bot:

[mediawiki/core@master] ParserOutput: Use page language instead of site content language for conversion

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

Change 768850 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] parser: Parse ToC along with page contents

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

Language conversion in ParserOutput didn't respect to <nowiki> tags and manual conversion rules defined at the top of the page.

Tag this as MW-1.38-release since it would be better to last the parser that provides correct parser caches in the release. Otherwise, it would be painful to do it in later versions.

Change 769116 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] Ensure that ToC is converted into the proper target language

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

T295091 is interesting. Whatever parsing needs to be done to fix that needs to be done *before* we extract the headings to make the table of contents, since the half-parsed content otherwise ends up in the JSON section data as well. In the next release I hope to remove getTOCHTML entirely and generate the TOC in :getText from the section contents in all code paths, so it is pretty important that the right data end up there. It may be that whatever tricks <math> is currently doing need to be adjusted (or math content stripped from headings).

Change 768850 abandoned by Func:

[mediawiki/core@master] parser: Parse ToC along with page contents

Reason:

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

Change 769116 merged by jenkins-bot:

[mediawiki/core@master] Ensure that ToC is converted into the proper target language

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

Change 771822 had a related patch set uploaded (by Arlolra; author: Subramanya Sastry):

[mediawiki/services/parsoid@master] Sync parser tests files with core

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

Change 771822 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Sync parser tests files with core

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

Change 772884 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.16.0-a2

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

Change 772884 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.16.0-a2

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

I don't know Chinese, but it appears to be fixed.

https://zh.wikipedia.org/zh-cn/%E4%BB%BB%E5%A4%A9%E5%A0%82Switch
https://zh.wikipedia.org/zh-hk/%E4%BB%BB%E5%A4%A9%E5%A0%82Switch

The first heading of these is different in each variant (first character is slightly different), and the TOC reflects that difference in its link labels, and it its link target also jumps correctly to the heading in question.

Confirm this issue is fixed now.

Func removed a project: Patch-For-Review.

Sadly we have a remaining conversion problem on T303855, but this should be fine to close.