Page MenuHomePhabricator

Replace usage of VirtualRESTServiceClient in Math extension
Open, LowPublic

Description

The Math MediaWiki extension makes use of the 3 endpoints exposed by mathoid service here: https://wikimedia.org/api/rest_v1/#/Math. Mathoid uses RESTBase for caching purposes.

1. /media/math/check/{type} - POST method
2. /media/math/formula/{hash} - GET method
2. /media/math/render/{format}/{hash} - GET method

We want to deprecate and remove the VirtualRESTService class. So, we will be using HTTP request objects in Math to access RESTBase and hence, Mathoid. In line with the RESTBase Sunsetting work currently going on and T333536, we'll be replacing usage of the VirtualRESTServiceClients with HTTP request objects for most if not all extensions that use it.

In the Math extension, specifically in MathRestbaseInterface.php class, we want to construct HTTP request objects for ::getContentRequest(), ::getCheckRequest() (for example) and use that to make the requests. Taking ::getContentRequest() for example, instead of returning an array with information about the request, we'll instead construct a MWHTTPRequest object and make a request like:

$request = MediaWikiServices::getInstance()->getHttpRequestFactory()->get(
	$this->getUrl( "media/math/render/$type/{$this->hash}" )
);
NOTE: Inject the HTTPRequestFactory into the interface.

For all occurrences of where the ::getServiceClient() method is called, we need to survey callers and appropriately use an HTTP request object and make a request directly to RESTBase using the URL mentioned above.

Acceptance Criteria

NOTE: All request must be executed in parallel, this is what $serviceClient->runMulti() it.
  • ::getCheckRequest() - replace this with parallel HTTP requests.
  • ::getContentRequest() - replace this with parallel HTTP request.
  • Retain the use of $wgVirtualRestConfig setting for backward compatibility
  • Delete the ::getServiceClient() method and references to where it's called
  • Ensure that the extension still works correctly for all endpoints exposed via Mathoid.

Event Timeline

DAlangi_WMF renamed this task from Replace usage of VirtualRESTServiceClient from Math extension to Replace usage of VirtualRESTServiceClient in Math extension.Apr 17 2023, 12:22 PM
DAlangi_WMF created this task.
daniel added a subscriber: Physikerwelt.

Please keep in mind that we need to remain compatible with existing configuration. So even if we no longer use VirtualRESTServiceClient , we need to still use $wgVirtualRestConfig, at least for now.

I think https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/913243/ actually covers all the subtasks we currently have on this, and also the acceptance criteria in the description.

The only thing left to do it deprecate the usage of wgVirtualRestConfig. But since it's not specific to this extension, that is a bit tricky...

Perhaps the best way to do that is to fix all the extensions that still use VirtualRestClient, and then deprecate wgVirtualRestConfig in core.

Once we land the "Revert "Revert" ..." patch and backport as discussed, then we can resolve this.

DAlangi_WMF updated the task description. (Show Details)

Backport deployed today. Thank you @daniel and @R_Rana. We'll monitor our sites as the train rolls to group 1 and 2. But group 0 works as expected.

I'll resolve this next week so we have a couple of days to see how things go when the code hits group 2 wikis.

Noted @Physikerwelt. I'll discuss this with Daniel and we'll see how to move this forward.

Thank you. Another comment. Selenium tests have failed since yesterday https://integration.wikimedia.org/ci/job/selenium-daily-beta-Math/

I've tried to setup selenium locally using fresh-node to reproduce this test case but it hasn't worked so far. I'm still trying but when I try the same formular <math>3 + 2</math>, I however see the rendering on the page and also, I can access the svg image as well. I'm not sure what causes this test to break but I'm still looking at it.

Thank you. Another comment. Selenium tests have failed since yesterday https://integration.wikimedia.org/ci/job/selenium-daily-beta-Math/

Ah, like the issue was that https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/919353/4 failed to merge. Now that it is in, the tests are green again. Good thing too, this would have broken production...

I agree with the idea outlined there. We'll work on a patch this week.

Ah, like the issue was that https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/919353/4 failed to merge. Now that it is in, the tests are green again. Good thing too, this would have broken production...

I can confirm that as of yesterday and today, https://integration.wikimedia.org/ci/job/selenium-daily-beta-Math/ is all green again.

Change 922166 had a related patch set uploaded (by D3r1ck01; author: Daniel Kinzler):

[mediawiki/extensions/Math@master] Introduce MathInternalRestbaseURL setting.

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

Change 922166 merged by jenkins-bot:

[mediawiki/extensions/Math@master] Introduce MathInternalRestbaseURL setting.

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

ReleaseTaggerBot added a project: MW-1.41-notes (1.41.0-wmf.12; 2023-06-06).

Should we keep this open and test again if the release has been rolled out, or should we close the task?

Should we keep this open and test again if the release has been rolled out, or should we close the task?

Maybe we can keep open until the change goes to atleast group 1 wikis next week? Generally, I would like to leave it for a week to make sure nothing goes wrong and then close afterwards but I'm open to your style too @Physikerwelt

I think it is better to keep the task open. Even better would be if there was an option to send out a reminder if the change goes to mediawiki.org.

I think it is better to keep the task open. Even better would be if there was an option to send out a reminder if the change goes to mediawiki.org.

The train rides to g0 wikis tomorrow (06/06/2024) and mediawiki.org is in g0. So by Wednesday morning, we should know if everything is fine then we can resolve this. Thanks!

DAlangi_WMF changed the task status from Open to In Progress.Jun 5 2023, 8:14 AM
DAlangi_WMF claimed this task.

Awesome. You beat me to it. I wanted to check it soon :). Thanks for verifying, I also checked the links you shared and it works for me too. Let's resolve this task now.

Finally, Math is free from VirtualRESTService <3

Caenus subscribed.

I will reopen this issue, all formulas is broken, see https://m.wikidata.org/wiki/User:Physikerwelt#Test and T338381

Change 928182 had a related patch set uploaded (by Physikerwelt; author: Physikerwelt):

[mediawiki/extensions/Math@master] Remove additional v1 when computing internalRestbaseURL

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

Change 928182 merged by jenkins-bot:

[mediawiki/extensions/Math@master] Remove additional v1 suffix when computing internalRestbaseURL

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

Change 928158 had a related patch set uploaded (by Samtar; author: Physikerwelt):

[mediawiki/extensions/Math@wmf/1.41.0-wmf.12] Remove additional v1 suffix when computing internalRestbaseURL

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

Change 928158 merged by jenkins-bot:

[mediawiki/extensions/Math@wmf/1.41.0-wmf.12] Remove additional v1 suffix when computing internalRestbaseURL

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

Mentioned in SAL (#wikimedia-operations) [2023-06-08T02:44:49Z] <samtar@deploy1002> Started scap: Backport for [[gerrit:928158|Remove additional v1 suffix when computing internalRestbaseURL (T334842 T338381)]]

Mentioned in SAL (#wikimedia-operations) [2023-06-08T02:46:33Z] <samtar@deploy1002> samtar: Backport for [[gerrit:928158|Remove additional v1 suffix when computing internalRestbaseURL (T334842 T338381)]] synced to the testservers: mwdebug2001.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug1002.eqiad.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-06-08T02:54:40Z] <samtar@deploy1002> Finished scap: Backport for [[gerrit:928158|Remove additional v1 suffix when computing internalRestbaseURL (T334842 T338381)]] (duration: 09m 50s)

I will reopen this issue, all formulas is broken, see https://m.wikidata.org/wiki/User:Physikerwelt#Test and T338381

Thanks for reporting @Caenus and for making a fix @Physikerwelt. Looks like this is resolved now, is it a good time to resolve the task? I'll leave it to either of you.

There is still one minor thing

I can confirm that as of yesterday and today, https://integration.wikimedia.org/ci/job/selenium-daily-beta-Math/ is all green again.

To me, it seems that this broke again. What did you do to make the tests green?

To me, it seems that this broke again. What did you do to make the tests green?

Looking at this: https://integration.wikimedia.org/ci/job/selenium-daily-beta-Math/ @Physikerwelt, this has been resolved?

From the console output here: https://integration.wikimedia.org/ci/job/selenium-daily-beta-Math/1756/console says something like [selenium-daily-beta-Math] $, meaning the failing test is run against beta cluster and since there has not been any patch into the Math repo since you reported this failure, it means the test fails when beta has an issue.

Can you confirm this? Also, we'll need to make the addition test give us reason for why it's failing so that we can know the actual reason.

To me, it seems that this broke again. What did you do to make the tests green?

Looking at this: https://integration.wikimedia.org/ci/job/selenium-daily-beta-Math/ @Physikerwelt, this has been resolved?

From the console output here: https://integration.wikimedia.org/ci/job/selenium-daily-beta-Math/1756/console says something like [selenium-daily-beta-Math] $, meaning the failing test is run against beta cluster and since there has not been any patch into the Math repo since you reported this failure, it means the test fails when beta has an issue.

Can you confirm this? Also, we'll need to make the addition test give us reason for why it's failing so that we can know the actual reason.

@TheresNoTime can you confirm? If I remember correctly, you submitted a patch to beta, which would explain the behavior.

DAlangi_WMF lowered the priority of this task from Medium to Low.Jun 17 2023, 7:23 PM

Removing inactive task assignee account. (Please do so as part of offboarding.)

@Physikerwelt, what's the status here? Seems @TheresNoTime needs to confirm something?

Note that production is still logging the following warning on pretty much every web request:

[Settings] WARNING: The MathInternalRestbaseURL is falling back to VirtualRestConfig. Please set MathInternalRestbaseURL explicitly.