morebits: add Morebits.taskManager #1036

siddharthvp · 2020-07-12T12:30:42Z

New task manager class (first mentioned in #839 (comment)) to automatically determine the execution sequence of asynchronous functions and run them, given the list of tasks (functions returning promises) and their dependencies.

I'll try to incorporate this into the xfd module at the first opportunity. But how to proceed is a bit unclear at the moment due to conflicts posed by #911 and #1030 (#911 isn't necessary to promisify the code in xfd module, it can also be done just putting var def = $.Deferred() at the top of each function and returning it).

siddharthvp · 2020-07-12T12:36:15Z

Unit test for this available at https://gist.github.com/siddharthvp/8796b9f44a95a3d2fbaf2bb437397706.

The version used there also has some console logging so that you can see what is executing and when. The functions just contain a setTimeout that resolve after some period. By the way the names of the functions and the dependencies are from a real example.

This rather atypical kind of code, so I'm happy to answer any questions.

siddharthvp · 2020-07-12T16:27:28Z

Well, I see that are no current PRs for the prod module. So I went ahead and overhauled PROD to use Morebits.taskManager. Excited to note that this went smoothly.

This has been extensively tested. Would be great to see this merged soon-ish.

Quoting from the commit message:

No actual changes made to the code, other than moving things around and making the functions return a $.Deferred() object.

Issues resolved:

The code is now MUCH more easy to follow. All functionality is broken up into functions whose names say exactly what the function does, unlike before when we had a function called main() that does half of the things itself.
- Until now we were having lot of lines like pageobj.setCallbackParamters(params) or var params = pageobj.getCallbackParameters(), or in case of api objects wikipedia_api.params = params or var params = apiobj.params. The same object (not a copy of the object) was being passed around in every function. In effect, the object was a global. What I have done now is to put an end to the mishegoss and actually declare it as a global accessible from anywhere without unnecessary paperwork.
Fixes Don't post {{Old prod}} if prod tagging fails #726. {{Old prod}} is now only posted if the tagging was successful.
Fixes the unreported issue of author notification succeeding even if tagging was aborted (such as when there was already a prod tag, and the user chose not to endorse).

siddharthvp · 2020-07-13T03:33:54Z

This is now also in use on User:SD0001/GAR-helper.js as another example usage.

siddharthvp · 2020-07-17T13:09:22Z

I've decided to do away with the function that checks for circular dependencies. I grossly over-estimated the amount of complexity of the call chains we'd be having. It turns out that they are simple enough for a human to easily tell if there are any circular links without needing to use automation.

Reproducing the code here, should it of interest:

/**
 * Check if there are any cyclic dependencies.
 * We represent the tasks as the vertices of a directed graph whose edges
 * represent dependencies, then perform a depth-first traversal.
 * See [[Depth-first search]] for a background on how this works.
 *
 * XXX: Is this worth it?
 * Useful for testing and debugging but shouldn't be required in production code
 */
this.checkCircularDependencies = function() {
	var visited = new Set();
	var executed = new Set();
	var dfs = function(task) {
		visited.add(task);
		var circularLink;
		var deps = this.taskDependencyMap.get(task);
		for (var i = 0; i < deps.length; i++) {
			if (!visited.has(deps[i])) {
				if (circularLink = dfs(deps[i])) { // eslint-disable-line no-cond-assign
					return circularLink;
				}
			} else if (!executed.has(deps[i])) {
				return deps[i];
			}
		}
		executed.add(task);
	}.bind(this);
	this.taskDependencyMap.forEach(function(_, task) {
		var link;
		if (!visited.has(task)) {
			if (link = dfs(task)) { // eslint-disable-line no-cond-assign
				// link.name (which gives the name of the function) won't work in IE 11,
				// so you're on your own in finding the circular link
				throw new Error('circular dependency detected at ' + link.name);
			}
		}
	});
};

siddharthvp · 2020-07-17T13:11:09Z

Also added support for resolving tasks with value (which would be available as arguments for use by the dependent functions). Updated the unit tests.

Amorymeltzer

This seems nice! I think it's actually a pretty big change, if subtly so, but it's nifty.

Aside from the redirect location issue, one other issue is the timing of that redirection. The timer seems to kick off from the start, so if there's any hangup (slow internet/computer, waiting for confirmation) then the page redirects immediately, possibly before the final action visually updates. I haven't been able to trigger anything actually not going through, but prod is fairly simple so I'd worry about something like AfD. At the very least, there's minimal chance for review.

Amorymeltzer · 2020-08-13T10:47:33Z

morebits.js

+	 * @param {function[]} deps - its dependencies
+	 */
+	this.add = function(func, deps) {
+		this.taskDependencyMap.set(func, deps);


Seems silly to require tm.add(func, []), is there a reason not to default to empty?

Suggested change

this.taskDependencyMap.set(func, deps);

deps = deps || [];

this.taskDependencyMap.set(func, deps);

Wouldn't be better to make the empty list explicit? In any case, I won't be able to push any code changes for quite some time.

It feels natural to me to not have to provide anything if there're no dependencies. Clearly not the most critical concern!

Amorymeltzer · 2020-08-13T11:07:57Z

modules/twinkleprod.js

-		'prop': 'templates',
-		'tltemplates': blocking_templates
-	};
+	Morebits.wiki.actionCompleted.redirect = mw.config.get('wgPageName');


Not entirely sure where/why, but this is all messed up? I'm getting redirected from /wiki/PageName to /wiki//wiki/PageName

Actually, that's only when not notifying; when notifying, it adds another, redirecting to /wiki//wiki//wiki/PageName

I don't recall coming across this in my testing. Morebits.wiki.actionCompleted.* is a messy system (basically depends on race conditions) which we probably don't want to use at least in places where taskManager is being used. I would suggest putting

location.href = mw.util.getUrl( mw.config.get('wgPageName') );

in the execute's then block. This method should be more reliable and clearer.

I haven't had much time so have been adding/tweaking a bunch of jsdoc stuff since I can do it in 30 second chunks in the morning and, wow, actionCompleted is entirely inscrutable to me. I can try and take a look in September if you don't have a chance, but, oof, there be dragons.

Amorymeltzer · 2020-08-13T11:54:57Z

modules/twinkleprod.js

@@ -174,205 +174,231 @@ Twinkle.prod.callback.prodtypechanged = function(event) {
 	event.target.form.replaceChild(field.render(), $(event.target.form).find('fieldset[name="parameters"]')[0]);
 };

+// global params object, initially set in evaluate(), and
+// modified in various callback functions
+var params = {};


Sorry for all the pushes, I missed this initially. I suppose it's okay here, but I think we'd want to be careful about expanding this elsewhere. The good news is it's how I found #1102!

Since wikimedia-gadgets#1093 (7a450cb), in an attempt to avoid pointless errors leading to *actual* errors, I moved the `pageobj.triage()` call to after the `Twinkle.xfd.currentRationale = null` limits such errors. This, however, meant that since then, AfD nominations were always trying to triage the *nomination* page, not the *nominated* page. This fixes that, although like wikimedia-gadgets#1093, in a somewhat kludgy way; anywhere else and we risk the page nomination resolving first (wikimedia-gadgets#1036). Increasingly thinking the original version of wikimedia-gadgets#930 would be the cleaner way to handle this.

No acutal changes made to the code, other than moving things around and making the functions return a $.Deferred() object. Issues resolved: - The code is now MUCH more easy to follow. All functionality is broken up into functions whose names say exactly what the function does, unlike before when we had a function called main() that does half of the things itself. - Until now we were having lot of lines like `pageobj.setCallbackParamters(params)` or `var params = pageobj.getCallbackParameters()`, or in case of api objects `wikipedia_api.params = params` or `var params = apiobj.params`. The same object (not a copy of the object) was being passed around in every function. In effect, the object was a global. What I have done now is to put an end to the mishegoss and actually declare it as a global accessible from anywhere without unnecessary paperwork. - Fixes wikimedia-gadgets#726. {{Old prod}} is now only posted if the tagging was successful. - Fixes the unreported issue of author notification succeeding even if tagging was aborted (such as when there was already a prod tag, and the user chose not to endorse).

in favour of doing the redirection in the then callback of execute(). Also ensure taggingPage() is done before doing addOldProd(), to avoid adding {{old prod}} if tagging was not done in the first place.

siddharthvp · 2020-10-17T21:25:42Z

Fixed the issue (by not using Morebits.wiki.actionCompleted setup). All seems good now.

Amorymeltzer · 2020-10-18T10:33:31Z

As above, this seems fine, and the taskManager is neat, but there are some things I don't think should be duplicated excessively, especially the subtle deprecation of Morebits.wiki.actionCompleted

siddharthvp force-pushed the taskmanager branch from 359c9cb to 739d7b4 Compare July 12, 2020 16:27

siddharthvp mentioned this pull request Jul 17, 2020

speedy: too slow #1051

Open

siddharthvp force-pushed the taskmanager branch from 739d7b4 to 84d23f9 Compare July 17, 2020 13:09

Amorymeltzer force-pushed the taskmanager branch 3 times, most recently from 31640d1 to ea27468 Compare August 13, 2020 11:53

Amorymeltzer requested changes Aug 13, 2020

View reviewed changes

Amorymeltzer added bug Feature request Module: morebits The morebits.js library Module: prod labels Aug 13, 2020

Amorymeltzer force-pushed the taskmanager branch from ea27468 to 952130a Compare August 14, 2020 11:45

Amorymeltzer added this to the October 2020 update milestone Sep 5, 2020

siddharthvp mentioned this pull request Oct 9, 2020

i18n/l10n roadmap #1144

Open

Amorymeltzer mentioned this pull request Oct 9, 2020

xfd: Fix AfD bug attempting to triage wrong page #1145

Merged

Amorymeltzer removed this from the October 2020 update milestone Oct 17, 2020

siddharthvp added 3 commits October 18, 2020 02:52

morebits: add Morebits.taskManager

d9794e4

prod: disable Morebits.wiki.actionCompleted

ee53737

in favour of doing the redirection in the then callback of execute(). Also ensure taggingPage() is done before doing addOldProd(), to avoid adding {{old prod}} if tagging was not done in the first place.

siddharthvp force-pushed the taskmanager branch from 952130a to ee53737 Compare October 17, 2020 21:24

siddharthvp merged commit 01fc94c into wikimedia-gadgets:master Oct 18, 2020

siddharthvp deleted the taskmanager branch October 18, 2020 05:38

Amorymeltzer added this to the October 2020 update milestone Oct 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

morebits: add Morebits.taskManager #1036

morebits: add Morebits.taskManager #1036

siddharthvp commented Jul 12, 2020

siddharthvp commented Jul 12, 2020

siddharthvp commented Jul 12, 2020

siddharthvp commented Jul 13, 2020

siddharthvp commented Jul 17, 2020

siddharthvp commented Jul 17, 2020

Amorymeltzer left a comment

Amorymeltzer Aug 13, 2020

siddharthvp Aug 13, 2020

Amorymeltzer Aug 13, 2020 •

edited

Loading

Amorymeltzer Aug 13, 2020

Amorymeltzer Aug 13, 2020

siddharthvp Aug 13, 2020

Amorymeltzer Aug 13, 2020

Amorymeltzer Aug 13, 2020 •

edited

Loading

siddharthvp commented Oct 17, 2020

Amorymeltzer commented Oct 18, 2020

	this.taskDependencyMap.set(func, deps);
	deps = deps \|\| [];
	this.taskDependencyMap.set(func, deps);

morebits: add Morebits.taskManager #1036

morebits: add Morebits.taskManager #1036

Conversation

siddharthvp commented Jul 12, 2020

siddharthvp commented Jul 12, 2020

siddharthvp commented Jul 12, 2020

siddharthvp commented Jul 13, 2020

siddharthvp commented Jul 17, 2020

siddharthvp commented Jul 17, 2020

Amorymeltzer left a comment

Choose a reason for hiding this comment

Amorymeltzer Aug 13, 2020

Choose a reason for hiding this comment

siddharthvp Aug 13, 2020

Choose a reason for hiding this comment

Amorymeltzer Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Amorymeltzer Aug 13, 2020

Choose a reason for hiding this comment

Amorymeltzer Aug 13, 2020

Choose a reason for hiding this comment

siddharthvp Aug 13, 2020

Choose a reason for hiding this comment

Amorymeltzer Aug 13, 2020

Choose a reason for hiding this comment

Amorymeltzer Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

siddharthvp commented Oct 17, 2020

Amorymeltzer commented Oct 18, 2020

Amorymeltzer Aug 13, 2020 •

edited

Loading

Amorymeltzer Aug 13, 2020 •

edited

Loading