Page MenuHomePhabricator

New phan dependencies significantly slowed down CI tests
Open, HighPublic0 Estimate Story Points

Description

rCICF27c11342c4a1: [EventBus] Add dependency seems like a very innocent change, adding in a phan dependency upon CentralNotice. However, we use the same dependency list (recursively) for PHPUnit tests as well, so that change added 64! new dependencies for EventBus, including some very slow ones like Wikibase and Scribunto.

This is much hunch as to what slowed down CI recently, since many of the extensions dependency maps intersect, we're pulling in basically everything. Likely we need to split the phan and phpunit/etc. dependency maps (keeping in mind T193824 as well).

Details

Related Gerrit Patches:

Event Timeline

Legoktm triaged this task as High priority.Jun 5 2019, 4:16 PM
Legoktm created this task.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 5 2019, 4:16 PM

It is possible to add a stub for the class from CentralNotice, so feel free to revert the config when the repository get's to heaver with the dependencies

Yeah, @Jdforrester-WMF suggested that on IRC as one way to go. Do you have an opinion on potentially splitting the dependency lists?

In general I'm not a huge fan of stubs because they require duplication, can get out of date, and then aren't true integration tests.

Daimona added a subscriber: Daimona.Jun 5 2019, 8:36 PM

I'm not a fan of stubs either. I think splitting PHPUnit and phan is the right way to do this. Temporarily using stubs is also fine, of course.

Can we revert this until this is fixed? There seem to be other problems than just slow CI; CentralNotice tests are failing when triggered by EventBus patches: https://phabricator.wikimedia.org/T225195#5240388

Can we revert this until this is fixed? There seem to be other problems than just slow CI; CentralNotice tests are failing when triggered by EventBus patches: https://phabricator.wikimedia.org/T225195#5240388

This is a general task, not focused on EventBus - it just happened to be one of the worst cases I noticed. It's probably best to deal with the failing tests on the other ticket.

It is blocking quite a few EventBus merges. Not sure how to get CentralNotice maintainers involved.

It is blocking quite a few EventBus merges. Not sure how to get CentralNotice maintainers involved.

I'm just saying it's better to focus discussion about that on T225195: EventBus jobs failing heavily because of CentralNotice and WikibaseRepo, not this ticket.

To get a overview about previous added dependency for phan - visit https://gerrit.wikimedia.org/r/#/q/status:merged+project:integration/config+topic:dependency


When splitting dependency for phan and phpunit, than the phan part must not resolve the dependency recursively, because the dependencies should be there explicit and not relay on dependency of another extension.
At the moment the inclusion of extensions/skins in .phan/config.php is not readable by a machine.

Having a requires.json (or similar) file in .phan folder could be read by an utility class in mediawiki-phan-config and than added to the directory_list and exclude_analysis_directory_list and it could be read by CI to get new required extensions directly cloned.

But I have no idea if that is a workable order for CI (first cloning the extension, read from it and than clone other extensions/skins).
It also splitts a bit the config file of phan and may make it more magic, but it json is in the same folder.

Change 516065 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[integration/config@master] Create a separate list of phan dependencies

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

Change 516066 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[integration/config@master] Drop dependencies from normal map added for phan

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

Change 516065 merged by jenkins-bot:
[integration/config@master] Create a separate list of phan dependencies

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

Change 516066 merged by jenkins-bot:
[integration/config@master] Drop dependencies from normal map added for phan

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

To get a overview about previous added dependency for phan - visit https://gerrit.wikimedia.org/r/#/q/status:merged+project:integration/config+topic:dependency

Thanks, this was super useful.

When splitting dependency for phan and phpunit, than the phan part must not resolve the dependency recursively, because the dependencies should be there explicit and not relay on dependency of another extension.
At the moment the inclusion of extensions/skins in .phan/config.php is not readable by a machine.
Having a requires.json (or similar) file in .phan folder could be read by an utility class in mediawiki-phan-config and than added to the directory_list and exclude_analysis_directory_list and it could be read by CI to get new required extensions directly cloned.

Hmm, this sounds like a good idea. We have a new "dev-requires" in extension.json that might work, needs a little adapting if we want the split lists.

But I have no idea if that is a workable order for CI (first cloning the extension, read from it and than clone other extensions/skins).
It also splitts a bit the config file of phan and may make it more magic, but it json is in the same folder.

See T211702: Quibble initialize step should only clone the target repository.

hashar added a comment.EditedJun 12 2019, 1:09 PM

We really need to phase out the list of dependencies being maintained in integration/config.git. Adding phan dependencies on top (via d371e28862ab9cd4d41c847cfdf643b4ad368895) adds more tech debt which we will have to repay soon.

I have noticed that most repositories already specify the phan dependencies in the config file (via directory_list and exclude_analysis_directory_list). An example for PageTriage which for which Phan depends on Echo and ORES

PageTriage/.phan/config.php
<?php
$cfg = require __DIR__ . '/../vendor/mediawiki/mediawiki-phan-config/src/config.php';
$cfg['directory_list'] = array_merge(
	$cfg['directory_list'],
	[
		'cron',
		'tools',
		'../../extensions/Echo',
		'../../extensions/ORES',
	]
);
$cfg['exclude_analysis_directory_list'] = array_merge(
	$cfg['exclude_analysis_directory_list'],
	[
		'../../extensions/Echo',
		'../../extensions/ORES',

	]
);

return $cfg;

Seems like we could declare them in extension/json or some other config file and let mediawiki/mediawiki-phan-config read that?

[snip]

That's exactly what @Umherirrender proposed a few comments above... (T225112#5241339). But yeah, I think that's doable once T211702 is done.

I noticed we already have a feature flag to disable recursion when the job name contains -phan-. Then I am not sure why we need to have a separate list for the Phan dependencies? If that is to add missing ones, we could just add them to the dependencies table?

Note T211702: Quibble initialize step should only clone the target repository is merely for the linters that do not have any other dependencies beside the source repository itself. It is merely a proposal to have a shortest path to failure, but we need to enhance Quibble to have dependencies between its tasks/stages.