Bug 49773 – nrwt multiprocessing - clean up test sharding a bit, add --workers-are flag

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

nrwt multiprocessing - clean up test sharding a bit, add --workers-are flag

[Dirk Pranke]

Created attachment 74329 [details]
Patch

[Dirk Pranke]

Created attachment 74330 [details]
fix typos

[Dirk Pranke]

Created attachment 74333 [details]
two more minor omissions

[Dirk Pranke]

Created attachment 74453 [details]
merge to tip of tree (r72458)

[Tony Chang]

Comment on attachment 74453 [details]
merge to tip of tree (r72458)

r- only for not validating possible values of workers-are.  The rest is just bikeshedding about naming, but I think we can come up with something better than workers_are.

View in context: https://bugs.webkit.org/attachment.cgi?id=74453&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:498
> +    def _shard_tests(self, test_files, single_threaded):

Maybe single_group instead of single_threaded since this is True if experiment_fully_parallel is true?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:586
> -        """Returns whether we should run all the tests in the main thread."""
> +        """Returns whether to pluralize log messages about DRT instances."""

The new docstring is confusing to me.  I'm not sure this function even needs a docstring.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1591
> +        optparse.make_option("--workers-are", action="store",
> +                             default="threads",
> +                             help="controls worker model. Valid values are "
> +                             "'inline' and 'threads' (default)."),

As mentioned in bug 49779, I think workers-are is a weird variable name.  I think concurrency-{mode,type,model} would be more descriptive, but hard to type as a command line flag.  Maybe --concurrency=inline (or single) and --concurrency=threads isn't too bad. The variable could still be concurrency_mode.  Maybe worker_mode, but that's pretty vague too.

To be clear, this means you could have flags for inline and experimental-fully-parallel?  Maybe experiment-fully-parallel should be rolled into this enum to avoid conflicting flags (separate change, of course)?

Also, can we check this somewhere (i.e., raise an exception if it's not inline or threads)?  It looks like you can add custom types to optparse, but that might not be worth it.

[Dirk Pranke]

(In reply to comment #5)
> View in context: https://bugs.webkit.org/attachment.cgi?id=74453&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:498
> > +    def _shard_tests(self, test_files, single_threaded):
> 
> Maybe single_group instead of single_threaded since this is True if experiment_fully_parallel is true?
>

It's actually one test per group, so N groups, not one. Although it turns out that we now lump all of the http and websocket tests into one group even in --experimental-fully-parallel mode,  which sucks.

I've changed the parameter name to "use_real_shards", which is maybe a bit more descriptive, and updated the docstring.

> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:586
> > -        """Returns whether we should run all the tests in the main thread."""
> > +        """Returns whether to pluralize log messages about DRT instances."""
> 
> The new docstring is confusing to me.  I'm not sure this function even needs a docstring.
> 

Agreed. I've renamed to this to _num_workers() and made it return the number instead of a boolean.

> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1591
> > +        optparse.make_option("--workers-are", action="store",
> > +                             default="threads",
> > +                             help="controls worker model. Valid values are "
> > +                             "'inline' and 'threads' (default)."),
> 
> As mentioned in bug 49779, I think workers-are is a weird variable name.

I've changed this to --worker-model and added validation tests.

[Dirk Pranke]

Created attachment 74621 [details]
update w/ review feedback, including renaming --workers-are to --worker-model

[Tony Chang]

Comment on attachment 74621 [details]
update w/ review feedback, including renaming --workers-are to --worker-model

View in context: https://bugs.webkit.org/attachment.cgi?id=74621&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:-605
> -        # FIXME: We should use webkitpy.tool.grammar.pluralize here.

Nit: Seems like we should keep this comment, no?

[Dirk Pranke]

(In reply to comment #8)
> (From update of attachment 74621 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74621&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:-605
> > -        # FIXME: We should use webkitpy.tool.grammar.pluralize here.
> 
> Nit: Seems like we should keep this comment, no?

IMO, no. That routine is way simplistic and only handles adding an "s" onto the end of a string. I don't think it adds enough value to be worth using.

[Adam Barth]

Comment on attachment 74621 [details]
update w/ review feedback, including renaming --workers-are to --worker-model

View in context: https://bugs.webkit.org/attachment.cgi?id=74621&action=review

>>> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:-605
>>> -        # FIXME: We should use webkitpy.tool.grammar.pluralize here.
>> 
>> Nit: Seems like we should keep this comment, no?
> 
> IMO, no. That routine is way simplistic and only handles adding an "s" onto the end of a string. I don't think it adds enough value to be worth using.

The idea is to route all these calls through that API so we can improve our pluralization handling in one place instead of having it copy/pasted around the codebase.

[Dirk Pranke]

(In reply to comment #10)
> (From update of attachment 74621 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74621&action=review
> 
> >>> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:-605
> >>> -        # FIXME: We should use webkitpy.tool.grammar.pluralize here.
> >> 
> >> Nit: Seems like we should keep this comment, no?
> > 
> > IMO, no. That routine is way simplistic and only handles adding an "s" onto the end of a string. I don't think it adds enough value to be worth using.
> 
> The idea is to route all these calls through that API so we can improve our pluralization handling in one place instead of having it copy/pasted around the codebase.

Well, sure. But it's not at all clear to me that building a generic pluralization routine is worth it for the types of code we're writing in webkitpy, and without having a generic, sufficiently flexible scheme, IMO using this routine some places and not others is just confusing.

I am actually speaking from experience here - I tried to use pluralize, and I even tried to modify pluralize to be generic enough to be useful for the places I would want to use it, and it was turning out to be way more work than just handling the pluralization directly.

[Adam Barth]

Comment on attachment 74621 [details]
update w/ review feedback, including renaming --workers-are to --worker-model

View in context: https://bugs.webkit.org/attachment.cgi?id=74621&action=review

>>>>> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:-605
>>>>> -        # FIXME: We should use webkitpy.tool.grammar.pluralize here.
>>>> 
>>>> Nit: Seems like we should keep this comment, no?
>>> 
>>> IMO, no. That routine is way simplistic and only handles adding an "s" onto the end of a string. I don't think it adds enough value to be worth using.
>> 
>> The idea is to route all these calls through that API so we can improve our pluralization handling in one place instead of having it copy/pasted around the codebase.
> 
> Well, sure. But it's not at all clear to me that building a generic pluralization routine is worth it for the types of code we're writing in webkitpy, and without having a generic, sufficiently flexible scheme, IMO using this routine some places and not others is just confusing.
> 
> I am actually speaking from experience here - I tried to use pluralize, and I even tried to modify pluralize to be generic enough to be useful for the places I would want to use it, and it was turning out to be way more work than just handling the pluralization directly.

Maybe the best thing for now is to leave the FIXME and someone can come by later and fix it?  If we were writing in ruby-on-rails, we'd use the built-in pluralize function because it's pretty awesome.  Someday, it would be nice to have an equally awesome pluralize function, either via some library or by writing it ourselves.

[Dirk Pranke]

> Maybe the best thing for now is to leave the FIXME and someone can come by later and fix it?  If we were writing in ruby-on-rails, we'd use the built-in pluralize function because it's pretty awesome.  Someday, it would be nice to have an equally awesome pluralize function, either via some library or by writing it ourselves.

I agree that RoR's pluralize is kind of cool in a "gee-whiz" way, but AFAIK even the advocates of that particular feature will agree that it only works in some cases and is not intended to work for everything everywhere. It only really works if you're just trying to pluralize a noun.

Not to put on my Old Man Hat or anything, but back in the day when I worked at Oracle -- where they used to take language translation deadly seriously and every log message that your code might output needed to be customizable -- they had a hard rule that singular and plural forms of messages needed to actually be two different strings, because printing intelligible messages just isn't as simple as pluralizing a noun.

The particular example that stopped me in this code is in the next diff block (paraphrasing):

if num_workers == 1:
   print_config("Running one DRT")
else:
   print_config("Running %d DRTs in parallel" % num_workers)

Not only do these message shift from spelled-out to numeric indicators, but it adds additional text onto the end. I use "one" because it's more readable than "1" in most cases. I claim that writing a pluralization routine that will do the right thing here is going to be more trouble than it's worth.

And, given the above, I don't want to use pluralization in one place and not in others in the same file, especially for what is virtually the same message. Which is not necessarily to say that we should never have a pluralize function, or that other code shouldn't be able to use it for other situations.

Given all that, the point of me removing the FIXME is that I did look into this, and decided that I didn't want to do it. It doesn't seem like leaving FIXMEs scattered through the code in case we decide to change decisions is really the way we write code.

Of course, one could argue that I shouldn't have deleted this line in a seemingly unrelated patch, but that's a whole different argument :)

[Dirk Pranke]

I apologize if my previous comments were overly harsh. Long day yesterday ... let me try again.

RoR's pluralize() method came about as a way to solve the problem that arises when you're trying build ORM tools, and you need to map between objects (conventionally named in the singular) and tables (conventionally plural). These things are typically single nouns, and the grammar doesn't have to be perfect but can usually be good enough, which is why it makes sense to implement it.

I don't think a pluralize()-type approach makes nearly as much sense for human-destined messaging. First, for the reasons above (the messages often differ in ways not easily handled by a simple routine), and second, because actually having the branch in the code is useful because then code coverage tools can highlight missed branches and it can help to identify test cases.

So, I don't think it makes a huge amount of sense to use the routine here, and I'd like to remove the comment.

[Dirk Pranke]

But it's hardly an issue worth arguing about ...

[Dirk Pranke]

Comment on attachment 74621 [details]
update w/ review feedback, including renaming --workers-are to --worker-model

Clearing flags on attachment: 74621

Committed r72641: <http://trac.webkit.org/changeset/72641>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

(In reply to comment #15)
> But it's hardly an issue worth arguing about ...

And in retrospect this afternoon, it's not actually unreasonable for that line of code, although I continue to think we shouldn't use it in general (e.g., in the other example).

I must've gotten up on the wrong side of the bed last night, or something. Sorry all.