This will entail a set of new mach commands as described at https://github.com/djmitche/taskcluster-in-tree-taskgraph and the supporting implementation. Ideally this comes with a kind that can handle the existing YAML files and produce an identical taskgraph, so we can switch to using it immediately before developing a greater variety of more flexible kinds.

Based on bug 1262165, :ahal and I chatted quickly about how to handle specification of the "target task set". I had initially suggested a "query language", then thought "oh, Python's a cool language, let's use that". But Andrew is working on frontends to try that do not involve optparse or any kind of human-writable string in a commit message. Instead, they involve people using tools, and those tools have access to data (either from running in-tree, or from looking up a decision task in the index). Andrew also pointed out that we sometimes want to parameterize tasks with user configuration: run these tests, enable this kind of logging, etc. This sort of thing would affect the task descriptions, not just selection of which tasks to run. So I think the better approach is for the taskgraph-generation code to take a list of task labels as its target specification. That list might be generated internally by a query over the full task set or externally. We can fold the existing parameters (repo url, revision, level, push id, message, blah blah blah) into the same data structure. So at a large level, the task-graph generation process would take as input: * a data structure including the above * a gecko tree * the index API (for optimization) and produce a graph as output. The data structure might be large and not especially human-readable, but is at least reproducible. The `mach taskgraph decision` command would actually *generate* (and log) this data structure, as it needs a simple templated way to accept a smaller set of parameters. So it would probably also be the thing either interpreting try option syntax, reading JSON from hg metadata, etc.

Can we have schema validation for the yaml files? This would be a huge plus, making them easier to maintain/update and also documenting their structure.

No, that's definitely off-topic here. The YAML files are going away.

This adds a new suite of mach commands, `mach taskgraph`, along with an implementation of a task-graph generation algorithm, tests, and documentation. Review commit: https://reviewboard.mozilla.org/r/50039/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50039/

This annotates all "legacy" task-graph jobs with attributes sufficient to allow filtering try jobs by attribute. Review commit: https://reviewboard.mozilla.org/r/50041/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50041/

This is a brand new parser for try syntax, based on the previous in-tree parser. Review commit: https://reviewboard.mozilla.org/r/50043/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50043/

Review commit: https://reviewboard.mozilla.org/r/50045/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50045/

Review commit: https://reviewboard.mozilla.org/r/50047/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50047/

This calls the TaskCluster API directly, rather than relying on the worker implementation to do so. Review commit: https://reviewboard.mozilla.org/r/50049/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50049/

Comment on attachment 8748839 [details] MozReview Request: Bug 1258497: allow mozunit to pass along kwargs; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50039/diff/1-2/

Comment on attachment 8748840 [details] MozReview Request: Bug 1258497: Factor out some common code; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50041/diff/1-2/

Comment on attachment 8748841 [details] MozReview Request: Bug 1258497: use `taskgraph decision` in decision task; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50043/diff/1-2/

Comment on attachment 8748842 [details] MozReview Request: Bug 1258497: Implement a new taskgraph generation system; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50045/diff/1-2/

Comment on attachment 8748843 [details] MozReview Request: Bug 1258497: Modify decision task to run 'taskgraph decision' Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50047/diff/1-2/

Comment on attachment 8748844 [details] MozReview Request: Bug 1258497: actually create tasks, using the taskcluster proxy Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50049/diff/1-2/

Wow, mozreview isn't quiet! I flagged just gps for review here, but I'd like a broader set of eyes so we can catch any serious design issues before they land. This is largely an implementation of the first step of https://github.com/djmitche/taskcluster-in-tree-taskgraph. Specifically, it sets out the well-defined task-graph generation process outlined in that proposal, and then hacks the existing "legacy" machinery to fit that mold. So it first creates the full task graph using try syntax `-b do -p all -u all`, attaching attributes to the tasks. It then filters the target task set using a try parser and those attributes. It doesn't yet implement optimization, and actually there's a bit more design work to do there. It finishes by calling queue.createTask for each resulting task. You can see the result in some try pushes above. So all in all, this should present no change for other Gecko hackers: everything keeps working the way it has. Aside from looking at try pushes and saying "huh, looks good", I've compared the generated tasks from the old and new systems, and found them to match. I've also compared the results of about 700 try messages, and only two differed. The differences had something to do with test platforms, and I couldn't make out what the old parser was doing with them; there's a decent chance it wasn't what the pushers expected! With this in place, we can start to broaden out the set of people hacking on this. It won't be me sitting in my corner writing code, telling everyone "stop messing with try and let me finish!". We can immediately start introducing new kinds, for one thing. We can start porting existing jobs into better-designed, non-legacy kinds. We can coordinate with mozilla-taskcluster to use the new big-graph scheduler. We can implement post-graph-generation filtering for things like --interactive or stripping caches from try builds. We can implement optimization, and start building tasks to do toolchain builds, make npm bundles, and so on. So I'd like feedback to focus on two things: * blockers to this landing in mozilla-inbound; and * substantial design problems we would have to carry around if this landed as-is The squashed patch is readable, but is also broken out into a few steps if you'd prefer smaller diffs.

https://reviewboard.mozilla.org/r/50037/#review47483 In essence I have no objections with it and sounds reasonable. Great patch! On another note, poor gps :'( I only skimmed the first half of the patch. ::: taskcluster/taskgraph/create.py:21 (Diff revision 2) > + > + for label in taskgraph.graph.visit_postorder(): > + task = taskgraph.tasks[label] > + deps_by_name = { > + n: label_to_taskid[r] > + for (l, r, n) in taskgraph.graph.edges Could you please have variable names that make it clear what they're? I can only figure out "l" wihtout looking at taskgraph.graph.edges. ::: taskcluster/taskgraph/generator.py:22 (Diff revision 2) > + instance construction time. The exception is `target_tasks`, which can be > + set at any time until `target_task_set` is accessed; this allows the target > + tasks to be determined based on `full_task_graph`. > + """ > + > + # Task-graph generation is implemented as a Python generator that yields Cool :) Very elegant. ::: taskcluster/taskgraph/generator.py:27 (Diff revision 2) > + # Task-graph generation is implemented as a Python generator that yields > + # each "phase" of generation. This allows some mach subcommands to short- > + # circuit generation of the entire graph by never completing the generator. > + > + def __init__(self, root, log, parameters, optimization_finder): > + self.root = root What does root represent in here? ::: taskcluster/taskgraph/graph.py:10 (Diff revision 2) > + > +import collections > + > +class Graph(object): > + """ > + Generic representation of a directed acyclic graph with labeled edges. Are edges the connection between two nodes? ::: taskcluster/taskgraph/graph.py:14 (Diff revision 2) > + """ > + Generic representation of a directed acyclic graph with labeled edges. > + Graph operations are implemented in a functional manner, so the data > + structure is immutable. > + > + It permits at most one edge of a given anme between any set of nodes. The s/anme/name/ ::: taskcluster/taskgraph/try_option_syntax.py:71 (Diff revision 2) > + 'web-platform-tests-reftests-e10s': re.compile(r'^web-platform-tests-reftests-e10s.*$'), > + 'xpcshell': re.compile(r'^xpcshell.*$'), > +} > + > +# We have a few platforms for which we want to do some "extra" builds, or at > +# least build-ish things. Sort of. Anyway, these other things are implemented You might be able to use the term generic (as generic tasks) or upstream tasks or root tasks if it helps.

I'm happy with the plan I read on the GH page. The patch also looks sound.

Review commit: https://reviewboard.mozilla.org/r/50833/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50833/

Review commit: https://reviewboard.mozilla.org/r/50835/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50835/

https://reviewboard.mozilla.org/r/50037/#review47549 I didn't look at the implementation details too much, but this seems like a really awesome way of solving the issues we've had with in-tree scheduling. Great work! ::: taskcluster/docs/attributes.rst:49 (Diff revision 3) > + * ``opt`` > + > +unittest_suite > +============== > + > +This is the unit test suite being run in a unit test task. This matches the For this, and also unittest_flavor, do you know what field this should map to for treeherder job submission? Looking at mozilla-taskcluster, as well as the new pulse schema [1] for job submission I do not see where we might send this in. [1] https://github.com/taskcluster/taskcluster-treeherder/blob/master/schemas/pulse-job.yml ::: taskcluster/docs/attributes.rst:68 (Diff revision 3) > +may not match either of ``unittest_suite`` or ``unittest_flavor``. > + > +test_chunk > +========== > + > +This is the chunk number of a chunked test suite (talos or unittest). Note Will this be included somewhere in task.extra that our integration component with treeherder can look at to inform it what chunk it is? Also, will there be a way to determine total chunks? ::: taskcluster/docs/taskgraph.rst:92 (Diff revision 3) > +* ``mach taskgraph tasks`` -- get the full task set > +* ``mach taskgraph full`` -- get the full task graph > +* ``mach taskgraph target`` -- get the target task set > +* ``mach taskgraph target-graph`` -- get the target task graph > +* ``mach taskgraph optimized`` -- get the optimized task graph > +* ``mach taskgraph decision`` -- run the whole task-graph generation process (expects to be run in a decision task) If this subcommand is expected to be run within a decision task, is there a way to generate the decision task output without submitting it to try or elsewhere? It appears there could be some useful artifacts produced for debugging/troubleshooting/comparing but you might not want to have it create a task for each when running locally. An example of a use case is making changes that could alter tasks within the graph and wanting to compare those before and after. Maybe there is a way of doing so that I haven't uncovered yet. ::: taskcluster/mach_commands.py:40 (Diff revision 3) > + def __call__(self, func): > + after = SubCommand.__call__(self, func) > + args = [ > + CommandArgument('--root', '-r', default='taskcluster/ci', > + help="root of the taskgraph definition relative to topsrcdir"), > + CommandArgument('--parameters', '-p', required=False, Because this file is not required, if you fail to specify certain keys that are expected to be present (such as project) when running a command such as `mach taskgraph full` you receive a key error rather than a message indicating what you could do to fix the issue (specify --project or a parameters file). It might be displaying “key error” is intentional, and if so, please just ignore what I said :) ::: taskcluster/mach_commands.py:119 (Diff revision 3) > + Decision task: generate a task graph and submit to TaskCluster. > + """)) > + @CommandArgument('--base-repository', > + default=os.environ.get('GECKO_BASE_REPOSITORY'), > + help='URL for "base" repository to clone ($GECKO_BASE_REPOSITORY)') > + @CommandArgument('--head-repository', Not sure if you also want to make this (as well as some of the other arguments) required since later when you attempt to get the key from the params dict, it will throw a key error. Such as line 78 in kind/legacy.py ::: taskcluster/taskgraph/kind/legacy.py:114 (Diff revision 3) > + 'day': pushdate[6:8], > + 'owner': params['owner'], > + 'level': params['level'], > + 'from_now': json_time_from_now, > + 'now': current_json_time(), > + 'revision_hash': params['revision_hash'] revision hash is not required when running the `decision` sub command, but will result in a key error here.

removing the NI?

dustin: It's been >24h since you submitted. I'll try to look at this first thing tomorrow morning.

> So I'd like feedback to focus on two things: > > * blockers to this landing in mozilla-inbound; and > * substantial design problems we would have to carry around if this landed > as-is > I don't have issues with any of these. Still looking at deeper level implementation. This is looking like the best configuration impl I've seen yet in terms of balancing flexibility with scalability.

https://reviewboard.mozilla.org/r/50039/#review47731 ::: taskcluster/mach_commands.py:56 (Diff revision 2) > + @Command('taskgraph', category="ci", > + description="Manipulate TaskCluster task graphs defined in-tree") > + def taskgraph(self): > + """The taskgraph subcommands all relate to the generation of task graphs > + for Gecko continuous integration. A task graph is a set of tasks linked > + by dependencies: for example, a binary must be built before it is tested, > + and that build may further depend on various toolchains, libraries, etc. > + """ This is fine, don't change anything.. but I have a general comment on using mach for this. I'm also likely the only person who feels this way :). I'm wary of commands that are 1) called by automation and 2) only used by a niche group of people. I'm not saying it's never ok, just to be deliberate about it. 1) Calling mach commands from (python) automation forces the API surface through argparse, which can become messy over time. Importing a library is much more flexible. 2) I can't imagine this command will be used much by people outside of releng/ateam. It seems like lower level kind of stuff that we should build easier to use tools on top of. That being said, I can see the convenience argument for people who do have to deal with taskgraphs. Just keep in mind it comes at the cost of a more confusing mach API. Finally, thanks for putting most of the logic in a separate taskgraph module! But one question, for the logic that does live here, why should it live here and not in taskgraph? I've found in the past when I ask myself that question, I don't have a good answer. Keep in mind mach commands can re-use an argument parser from its underlying module. My experience is when there is too much logic in a mach command, it tends to become a dumping ground because outside developers are more comfortable modifying a mach_commands.py than they are digging into an unfamiliar library. This is a general comment, not specifically directed at your patch. You did a good job keeping most of the logic out.

What ahal raises up in #1 is probably something I should care about. I would not be surprised if I wanted or needed in the future this to be released as a package I could use. I'm happy to re-factor later but it would be great if we took care of it now.

..and don't take any inputs from environment variables. This also makes `--parameters` mandatory for the non-decision taskgraph commands, since without it they fail. Review commit: https://reviewboard.mozilla.org/r/51143/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51143/

Review commit: https://reviewboard.mozilla.org/r/51145/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51145/

Comment on attachment 8748842 [details] MozReview Request: Bug 1258497: Implement a new taskgraph generation system; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50045/diff/2-3/

Comment on attachment 8748843 [details] MozReview Request: Bug 1258497: Modify decision task to run 'taskgraph decision' Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50047/diff/2-3/

Comment on attachment 8748844 [details] MozReview Request: Bug 1258497: actually create tasks, using the taskcluster proxy Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50049/diff/2-3/

Comment on attachment 8749286 [details] MozReview Request: Bug 1258497: support test platform pretty names; r?sfink Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50833/diff/1-2/

Comment on attachment 8749287 [details] MozReview Request: Bug 1258497: add partial tests for legacy kind; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50835/diff/1-2/

Comment on attachment 8749749 [details] MozReview Request: Bug 1258497: require all decision task arguments; r?garndt https://reviewboard.mozilla.org/r/51143/#review47835

Comment on attachment 8748839 [details] MozReview Request: Bug 1258497: allow mozunit to pass along kwargs; r=gps https://reviewboard.mozilla.org/r/50039/#review47875 From a high level, I like what I see. I'll have to look through the rest of the series to be sure. I apologize for the amount of low-level nits I flagged. I shouldn't have done that on the first pass. ::: moz.build:25 (Diff revision 2) > > DIRS += [ > 'config', > 'python', > 'testing', > + 'taskcluster', This should cause a build failure because it will require a `taskcluster/moz.build` file. If it doesn't trigger a build failure, you found a build system bug. ::: taskcluster/PARAMETERS.md:19 (Diff revision 2) > +* `base_repository` - the repository from which to do an initial clone, utilizing > + any available caching. > + > +* `head_repository` - the repository containing the changeset to be built. This > + may differ from `base_repository` in cases where `base_repository` is likely > + to be cached and only a few additional commits are needed from > + `head_repository`. > + > +* `head_rev` - the revision to check out; this can be a short revision string > + > +* `head_ref` - for Mercurial repositories, this is the same as `head_rev`. For > + git repositories, which do not allow pulling explicit revisions, this gives > + the symbolic ref containing `head_rev` that should be pulled from > + `head_repository`. > + > +* `revision_hash` - the full-length revision string I was never a fan of this terminology from tc-vcs: it assumes too much about tc-vcs's caching design. We can bikeshed this later. ::: taskcluster/PARAMETERS.md:45 (Diff revision 2) > +Tree Information > +---------------- > + > +* `project` - another name for what may otherwise be called tree or branch or > + repository. This is the unqualified name, such as `mozilla-central` or > + `cedar`. > + > +* `level` - the SCM level associated with this tree. This dictates the names > + of resources used in the generated tasks, and those tasks will fail if it > + is incorrect. More VCS terminology I'm not a fan of. "tree" doesn't mean anything in Mercurial speak. And with distributed version control systems a logical repository can exist at multiple URLs and multiple branches/refs/bookmarks within that. I'm not sure what else we should call this though. Since you use "target set" below, it's tempting to call this a "target" or "goal." Or maybe the inverse: "source" or "vcs_source." I'll have to think about this more. ::: taskcluster/mach_commands.py:202 (Diff revision 2) > + > + def load_parameters_file(self, options): > + filename = options['parameters'] > + if not filename: > + return {} > + file = open(filename) with open(filename, 'rb') as fh: ... ::: taskcluster/taskgraph/generator.py:40 (Diff revision 2) > + > + # start the generator > + self._run = self._run() > + self._run_results = {} > + > + @property I think this is @property abuse. I don't like using @property for things that are really complex functions. If nothing else, it makes things harder to extend later when you wish to pass arguments to control behavior. ::: taskcluster/taskgraph/generator.py:62 (Diff revision 2) > + """ > + return self._run_until('full_task_graph') > + > + def set_target_tasks(self, target_tasks): > + if 'target_task_set' in self._run_results: > + raise RuntimeError("target_task_set has already been generated; " What's wrong with "Exception" or inventing your own Exception-derived class? You don't typically see RuntimeError outside of the stdlib. ::: taskcluster/taskgraph/generator.py:107 (Diff revision 2) > + 'name': name, > + 'path': path, > + }, "loading kind `{name}` from {path}") > + > + kind_yml = os.path.join(path, 'kind.yml') > + config = yaml.load(open(kind_yml)) Always use `with` with `open()`. ::: taskcluster/taskgraph/generator.py:132 (Diff revision 2) > + def _run(self): > + all_tasks = {} > + for kind in self._load_kinds(): > + for task in kind.load_tasks(self.parameters): > + if task.label in all_tasks: > + raise KeyError("duplicate tasks with label " + task.label) Generally KeyError should only be raised by a container-like class during a key get/set operation. Stick with Exception or a custom class derived from it. ::: taskcluster/taskgraph/graph.py:14 (Diff revision 2) > + """ > + Generic representation of a directed acyclic graph with labeled edges. > + Graph operations are implemented in a functional manner, so the data > + structure is immutable. > + > + It permits at most one edge of a given anme between any set of nodes. The "anme" ::: taskcluster/taskgraph/graph.py:15 (Diff revision 2) > + graph is not checked for cycles, and methods may hang or otherwise fail if > + given a cyclic graph. That's scary. Can someone hacking on task definitions accidentally create a cycle leading to this badness? Since you aren't using recursive function calls, Python won't even blow out its stack to abort quickly :/ ::: taskcluster/taskgraph/graph.py:23 (Diff revision 2) > + The `nodes` and `edges` attributes may be accessed in a read-only fashion. > + The `nodes` attribute is a set of node names, while `edges` is a set of > + `(start, end, label)` tuples. > + """ > + > + __slots__ = ['nodes', 'edges'] __slots__ here is probably premature optimization. You generally only need __slots__ if you are creating thousands of instances of something. ::: taskcluster/taskgraph/graph.py:57 (Diff revision 2) > + add_edges = set((l, r, n) for (l, r, n) in self.edges if l in nodes) > + add_nodes = set(r for (l, r, n) in add_edges) Nit: you don't need the parens after the "for" ::: taskcluster/taskgraph/graph.py:92 (Diff revision 2) > + def links_dict(self): > + """ > + Return a dictionary mapping each node to a set of its downstream > + nodes (omitting edge names) > + """ > + links = collections.defaultdict(lambda: set()) defaultdict will call the thing passed to it. Since `set` can be called like `set()`, you can just use `collections.defaultdict(set)`. ::: taskcluster/taskgraph/kind/legacy.py:47 (Diff revision 2) > +DEFAULT_JOB_PATH = os.path.join( > + 'tasks', 'branches', 'base_jobs.yml' > +) > + > + > +class LegacyKind(base.Kind): docstring please! ::: taskcluster/taskgraph/kind/legacy.py:71 (Diff revision 2) > + if vcs_info: > + pushdate = time.strftime('%Y%m%d%H%M%S', time.gmtime(vcs_info.pushdate)) > + > + self.log(logging.DEBUG, 'vcs-info', {}, > + '%d commits influencing task scheduling:\n' % len(vcs_info.changesets)) > + for c in vcs_info.changesets: > + self.log(logging.DEBUG, 'vcs-relevant-commit', { > + 'cset': c['node'][0:12], > + 'desc': c['desc'].splitlines()[0].encode('ascii', 'ignore'), > + }, "{cset} {desc}") > + changed_files |= set(c['files']) Hmmm. All this code looks very familiar. Is your intent to refactor it so you don't copy it? ::: taskcluster/taskgraph/test/test_generator.py:54 (Diff revision 2) > + self.tgg = WithFakeKind('/root', log, {}, None) > + > + def test_full_task_set(self): > + "The full_task_set property has all tasks" > + self.assertEqual(self.tgg.full_task_set.graph, > + graph.Graph(set(['t-0', 't-1', 't-2']), set())) We require Python 2.7, so you can use set literals: `{'t-0', 't-1'}` ::: taskcluster/taskgraph/types.py:1 (Diff revision 2) > +import pprint Unused symbol. ::: taskcluster/taskgraph/types.py:32 (Diff revision 2) > + def __str__(self): > + return "{} ({})".format(self.task_id or self.label, > + self.task['metadata']['description'].strip()) You may also want to implement __repr__ which is what is shown inside Python debuggers. ::: taskcluster/taskgraph/types.py:45 (Diff revision 2) > + > + A task graph is a combination of a Graph and a dictionary of tasks indexed > + by label. TaskGraph instances should be treated as immutable. > + """ > + > + __slots__ = ['tasks', 'graph'] Probably premature optimization. ::: testing/taskcluster/taskcluster_graph/mach_util.py:1 (Diff revision 2) > +from __future__ import absolute_import You should really split the refactoring into a separate commit from the new code. Having everything in one giant commit makes things a bit harder to review.

Comment on attachment 8748840 [details] MozReview Request: Bug 1258497: Factor out some common code; r=gps https://reviewboard.mozilla.org/r/50041/#review47887 ::: taskcluster/TASK_ATTRIBUTES.md:1 (Diff revision 2) > +Tasks can be filtered, for example to support "try" pushes which only perform a This looks like ReST, not Markdown. ::: taskcluster/TASK_ATTRIBUTES.md:21 (Diff revision 2) > +The build platform defines the platform for which the binary was built. It is > +set for both build and test jobs, although test jobs may have a different > +`test_platform`. I thought you were going to get rid of the distinction between "build" and "test" tasks and make things more generic? It's hard to tell from this commit alone because you only change the legacy Kind...

Comment on attachment 8748841 [details] MozReview Request: Bug 1258497: use `taskgraph decision` in decision task; r=gps https://reviewboard.mozilla.org/r/50043/#review47895 I only glanced at this one. ::: taskcluster/taskgraph/try_option_syntax.py:2 (Diff revision 2) > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this file, Not sure where you copied this from, but "file," is supposed to be on the 3rd line. ::: taskcluster/taskgraph/try_option_syntax.py:23 (Diff revision 2) > +} > + > +# mapping from shortcut name (usable with -u) to a regexp matching a set of > +# test suites > +UNITTEST_ALIASES = { > + 'cppunit': re.compile(r'^cppunit.*$'), I've been taught by Perl gurus that `.*` in regular expressions is somewhat evil. In most of these patterns, you are doing `^foo.*$` which is equivalent to `s.startswith('foo')`. So what value does a regular expression provide? OK, it provides consistency. But AFAICT they all end in `.*$`. You aren't doing capture, so you don't need the `.*$` because it will almost certainly match anything. Also, `.*` after a literal text will continue matching literal text and many other characters! So e.g. `cppunit.*` will match `cppunits` `cppunit0` and so on. I don't think this is what you want.

Comment on attachment 8748842 [details] MozReview Request: Bug 1258497: Implement a new taskgraph generation system; r?gps https://reviewboard.mozilla.org/r/50045/#review47897 This looks mostly good. (I'm not marking anything r+ on this series just quite yet.)

Comment on attachment 8748843 [details] MozReview Request: Bug 1258497: Modify decision task to run 'taskgraph decision' https://reviewboard.mozilla.org/r/50047/#review47899 Nice!

Comment on attachment 8748844 [details] MozReview Request: Bug 1258497: actually create tasks, using the taskcluster proxy https://reviewboard.mozilla.org/r/50049/#review47901 Shouldn't this commit come before the one before (which switches to this mach command for generating the graph)? ::: taskcluster/taskgraph/create.py:33 (Diff revision 3) > + _create_task(label_to_taskid[label], task_def) > + > +def _create_task(task_id, task_def): > + # create the task using 'http://taskcluster/queue', which is proxied to the queue service > + # with credentials appropriate to this job. > + print("Creating task {}".format(task_id)) Should use a logger API instead, as this is a module, not a CLI program.

https://reviewboard.mozilla.org/r/50833/#review47903 ::: taskcluster/taskgraph/try_option_syntax.py:216 (Diff revision 2) > - tests = self.parse_test_opts(unittest_arg) > + all_platforms = set([t.attributes['test_platform'] > + for t in full_task_graph.tasks.itervalues() > + if 'test_platform' in t.attributes]) Nit: you don't need the [] when feeding a list comprehension into set().

Comment on attachment 8749287 [details] MozReview Request: Bug 1258497: add partial tests for legacy kind; r?gps https://reviewboard.mozilla.org/r/50835/#review47905 Will review this for real during the next pass.

https://reviewboard.mozilla.org/r/51143/#review47907 ::: taskcluster/mach_commands.py:124 (Diff revision 1) > @CommandArgument('--base-repository', > - default=os.environ.get('GECKO_BASE_REPOSITORY'), > - help='URL for "base" repository to clone ($GECKO_BASE_REPOSITORY)') > + required=True, > + help='URL for "base" repository to clone') > @CommandArgument('--head-repository', > - default=os.environ.get('GECKO_HEAD_REPOSITORY'), > - help='URL for "head" repository to fetch revision from ' > + required=True, > + help='URL for "head" repository to fetch revision from') I don't think we need to require both. You should only need `--head-repository`. `--base-repository` should only come into play for something like Try. Or is this a workaround to appease the tc-vcs caching design?

https://reviewboard.mozilla.org/r/51145/#review47909 You might as well fold this into the commit that introduces the code that was refactored. Should make review a bit easier since this includes unit tests.

https://reviewboard.mozilla.org/r/51145/#review47909 Yeah, it was added after the initial review request. I may fold the whole thing into one commit.

Comment on attachment 8749750 [details] MozReview Request: Bug 1258497: factor more code out of mach_commands.py; r?ahal https://reviewboard.mozilla.org/r/51145/#review48091 Thanks for doing this, looks great! ::: taskcluster/taskgraph/test/test_target_tasks.py:6 (Diff revision 1) > +import os > +import sys > +import json > +import yaml > +import shutil > +import unittest > +import tempfile > + > +from taskgraph import target_tasks > +from taskgraph import try_option_syntax > +from taskgraph.graph import Graph > +from taskgraph.types import Task, TaskGraph > + > +from mozunit import main, MockedOpen Most of these imports are unused.

Comment on attachment 8749286 [details] MozReview Request: Bug 1258497: support test platform pretty names; r?sfink https://reviewboard.mozilla.org/r/50833/#review48187 ::: taskcluster/taskgraph/test/test_try_option_syntax.py:194 (Diff revision 1) > + def test_u_platforms_negated(self): > + "-u gtest[-linux] selects no platforms for gtest" > + tos = TryOptionSyntax('try: -u gtest[-linux]', graph_with_jobs) > + self.assertEqual(sorted(tos.unittests), sorted([ > + {'test': 'gtest', 'platforms': []}, > + ])) This is a change from previous behavior, where it would mean all but linux. ::: taskcluster/taskgraph/try_option_syntax.py:257 (Diff revision 1) > + def normalize_platforms(): > + if 'platforms' not in cur_test: > + return > + platforms = [] > + for platform in cur_test['platforms']: > + if platform[0] == '-': > + platforms = [p for p in platforms if p != platform[1:]] > + else: > + platforms.append(platform) This differs from the original handling, which is not necessarily a problem, but the behavior here is a little weird. The platforms list is modified for each element in the list. Which means that [-X] matches nothing instead of the current "all but X". And there doesn't seem to be a magic 'all' token, so you can't achieve "all but X" with [all,-X]. And it appears you were thinking of this at some point, because that's the whole reason to pass in all_platforms. Currently, it is unused. The tricky thing is that the initial state depends on the set of test platforms. [-x] means start with all platforms and remove x, whereas [x] means start with nothing and add x. To fully match the previous behavior, you'd need [-x,y] to be the same as [y,-x] (as in, your initial state is all_platforms iff there are no non-negated platforms in the list). But it'd probably be enough to just set the initial value based on whether the first platform is negated or not. Or: afaik the whole restrictions thing is only used for T pushes. I think a reasonable option would be to remove support for restrictions on tests entirely, and add in a --test-platforms (and maybe --skip-test-platforms) option. It would certainly simplify all this stuff. But I guess I understand if you want to maintain backwards compatibility with the set of strings produced by the trychooser web UI. The full generality of "-u X[-a],Y[b,-a],Z[b]" definitely feels like overkill, though. It'd be interesting to see if anyone has ever done a push like that. I'd guess it's much more likely that someone has done "-u X,Y[x64]" and been surprised at getting test X on Windows. (The above is not the same as "-u X[x64],Y[x64]".) So limiting the whole test list to a single restriction and applying it equally to all tests might be an improvement.

https://reviewboard.mozilla.org/r/50043/#review48189 ::: taskcluster/taskgraph/try_option_syntax.py:78 (Diff revision 2) > + 'sm-generational', > + 'sm-rootanalysis', > + 'sm-warnaserr', sm-generational and sm-warnaserr are now dead. sm-arm64-sim should now be added in.

https://reviewboard.mozilla.org/r/50045/#review48191 ::: taskcluster/docs/old.rst:27 (Diff revision 3) > +In order to properly enable task reuse there are a small number of > +conventions and parameters that are specialized for build tasks vs test > +tasks. The goal here should be to provide as much of the power as > +taskcluster but not at the cost of making it easy to support the current > +model of build/test. (pre-existing, I think?) No, we certainly wouldn't want it to be easy, would we? :-) grammar nits: "there are a small number of" -> "there is a small number of" or "there are a few". "as much of the power as taskcluster" - I guess you meant "as much of the power of taskcluster as possible"? and I think you meant s/but not at the cost of/while still/ ::: taskcluster/docs/old.rst:39 (Diff revision 3) > +Helper for always using the latest version of a docker image that exist > +in tree:: exists. (Maybe "...that exists in the tree"?) ::: taskcluster/docs/old.rst:80 (Diff revision 3) > +Task format > +----------- > + > +To facilitate better reuse of tasks there are some expectations of the > +build tasks. These are required for the test tasks to interact with the > +builds correctly but may not effect the builds or indexing services. affect ::: taskcluster/docs/old.rst:89 (Diff revision 3) > + # Builders usually create at least two important artifacts the build > + # and the tests these can be anywhere in the task and also may have > + # different path names to include things like arch and extension Punctuatify! # Builders usually create at least two important artifacts: the build and the tests. These can be anywhere in the task, and also may have different path names to include things like arch and extension. What does "...can be anywhere in the task" mean? Anywhere in the directory tree for a task? ::: taskcluster/docs/old.rst:168 (Diff revision 3) > +root > + *optional* Boolean indicating whether this is a *root* task. Root > + tasks are scheduled immediately, if scheduled to run. What does this mean, exactly? Aren't all tasks scheduled immediately, as soon as all their prerequisites are available? What's the difference between being a root task and simply not having any prerequisites? Ok, I'm realizing that all of these things are pre-existing, so I'll shut up about them now. ::: taskcluster/docs/taskgraph.rst:21 (Diff revision 3) > +* *Task Labels* - Each task has a unique identifier within the graph that is > + stable across runs of the graph generation algorithm. Labels are replaced with > + TaskCluster TaskIds at the latest time possible, preventing spurious taskId > + changes in diffs made before that time. Sorry, what? This is probably my unfamiliarity with the whole setup here, but what are these diffs you speak of? Are you talking about for internal debugging, where you dump out the graph at various stages (pre- and post-optimization or something)? Oh! Ok, I read to the end, and this is now clear. Perhaps mention that the diffs are between outputs of the various mach taskgraph subcommands or something?

Feedback: I don't really know enough about the current setup to really comment on what I think of the improvements here, but these are my thoughts upon reading your design doc: Do tasks always run in separate environments, communicating only through artifacts? Or is there a way to have a followup task (presumably optional for some platforms or pushes or whatever, else it would be absorbed into the parent) that executes on the same filesystem? Still not sure about the difference between a root tasks and one with no prerequisite tasks. (Whatever it is, could it instead of represented by a dependency on the decision task, or perhaps on a dummy task representing a push, or something?) What is the 'assemble' kind in the example at https://github.com/djmitche/taskcluster-in-tree-taskgraph ? Back to this hobby horse -- periodic tasks feel unnecessarily different from decision tasks. Is there any way to pull them into the graph setup a little farther? It's unclear to me what the contents of .taskcluster.yml and .cron.yml are. Is there a way to request a specific artifact or something be generated, and have a component that gives you back a graph that will accomplish that (whatever it may take)? Is there any level of indirection, where a task can produce some virtual thing that other tasks could depend on? (Pulse messages are sorta kinda a virtual output, in that they trigger downstream non-taskcluster actions already.) Totally random use case that I'd like to handle semi-cleanly, somehow: hazard jobs produce some output that I use to populate a queriable database. It's expensive to import, so I want to import updated artifacts once a day. I guess with indexing, I could poll for the latest results, detect if it has changed more than 24 hours after my last update, and update if so. But I'd kind of like the task to know whether it's the daily update task, and produce some additional output if so. For this, I could manually spawn the task with added flags or a modified try push or whatever, but I'd prefer to pull existing results down to avoid introducing any load. (Especially for trees that aren't getting updated anymore; it'd be nice for them to naturally stop updating my downstream thing too.) Probably outside of the current redesign.

Good catch on the test platform errors - you're right, I started to implement that correctly and must have gotten distracted. And yes, the docs are copied, but I'll make the edits you suggest. Yes, tasks are always isolated. I have no idea what root tasks are -- greg added that, I think partially. The "assemble" kind is my fumbling for a term for the "fast" part of artifact builds: take a bunch of already-compiled stuff and mush it together to make a firefox.exe. Yes, I want to make periodic jobs much more similar to on-push jobs, by both running decision tasks with slightly different arguments. If you want a specific artifact, you would figure out the label of the task that produces that artifact, and then create a decision task specifying only that label as the target_task_set. I don't think there's any need for "virtual artifacts" -- the attributes already provide enough abstraction for that, allowing tasks of one kind to find prerequisite tasks of another kind without too much coupling. For your use-case, I think the index is best. I'd prefer not to introduce too much non-determinism into the task-graph generation process, as that makes it hard to compare results. For example, if someone adds a new test type, it'd be nice for a diff of graph-before-add and graph-after-add to show just the new test, and not the unexpected absence of a hazard result upload task.

I think I've incorporated all of the feedback and review above. Once this looks good in try, I'll push for another round of review. I've squashed (sorry, rolled) most of the csets together since they largely refactors of the same code, and only marginally conceptually separate.

Comment on attachment 8748839 [details] MozReview Request: Bug 1258497: allow mozunit to pass along kwargs; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50039/diff/2-3/

Comment on attachment 8748840 [details] MozReview Request: Bug 1258497: Factor out some common code; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50041/diff/2-3/

Comment on attachment 8748841 [details] MozReview Request: Bug 1258497: use `taskgraph decision` in decision task; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50043/diff/2-3/

Comment on attachment 8748842 [details] MozReview Request: Bug 1258497: Implement a new taskgraph generation system; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50045/diff/3-4/

Comment on attachment 8748839 [details] MozReview Request: Bug 1258497: allow mozunit to pass along kwargs; r=gps https://reviewboard.mozilla.org/r/50039/#review48889

Comment on attachment 8748840 [details] MozReview Request: Bug 1258497: Factor out some common code; r=gps https://reviewboard.mozilla.org/r/50041/#review48891 I didn't verify you didn't change any code. I trust you. ::: testing/taskcluster/taskcluster_graph/mach_util.py:1 (Diff revision 3) > +from __future__ import absolute_import A license header (I assume MPL) should be added.

Comment on attachment 8748842 [details] MozReview Request: Bug 1258497: Implement a new taskgraph generation system; r?gps https://reviewboard.mozilla.org/r/50045/#review48893 Submitting review for just the docs and mach_commands.py file. Will look at the Python code next... Also, the 3rd commit in this series should come after this one. I'll review that commit once I get through this one. ::: taskcluster/docs/attributes.rst:1 (Diff revision 4) > +=============== Try running `mach doc`. I think it will fail because there isn't a `taskcluster/docs/index.rst` file. ::: taskcluster/docs/attributes.rst:43 (Diff revision 4) > + * ``debug`` > + * ``opt`` What about PGO? What about e.g. static analysis builds? Are these different build platforms? Different build types? ::: taskcluster/docs/attributes.rst:46 (Diff revision 4) > +unittest_suite > +============== > + > +This is the unit test suite being run in a unit test task. > + > +unittest_flavor > +=============== While we're here, I'm trying to add job metrics to Perfherder in bug 1271077. One of the things that's tripping me up is figuring out what "key" to insert the data in under Perfherder. From mozharness, it isn't exactly trivial to come up with a proper key. In buildbot, the key is the builder name. However, those names are really long and wonky. It would be awesome if TC had a way to generate a deterministic "task identifier" constructed from the platform, type, flavor, chunk, etc. ::: taskcluster/docs/attributes.rst:49 (Diff revision 4) > + * ``opt`` > + > +unittest_suite > +============== > + > +This is the unit test suite being run in a unit test task. Example values would be nice. ::: taskcluster/docs/attributes.rst:67 (Diff revision 4) > +This is the chunk number of a chunked test suite (talos or unittest). Note > +that this is a string! It feels weird that everything is a string despite using YAML, which can preserve types. I'm generally a fan of strong typing where it can prevent bugs. I haven't looked at the code yet: hopefully the YAML loading code raises an error if it doesn't encounter a string in YAML. ::: taskcluster/docs/parameters.rst:9 (Diff revision 4) > + > +Task-graph generation takes a collection of parameters as input, in the form of > +a JSON or YAML file. > + > +During decision-task processing, some of these parameters are supplied on the > +command line or by environment variables. The decision task helpfully produces No style guide updated in the 21st century recommends double spaces after periods. This requirement stemmed from when typing was set by hand. I was taught to use double spaces in school too. I've since unlearned that practice :) ::: taskcluster/docs/taskgraph.rst:19 (Diff revision 4) > + tasks of other kinds. Kinds are the primary means of supporting diversity, in > + that a developer can add a new kind to do just about anything without > + impacting other kinds. > + > +* *Task Attributes* - Tasks have string attributes by which can be used for filtering. > + Attributes are documented in :doc:`attributes`. Does this ref work? I thought you needed to do e.g. :ref:`taskcluster_attributes` (which requires a `.. taskcluster_attributes:` at the top of attributes.rst to define that "anchor." ::: taskcluster/docs/taskgraph.rst:32 (Diff revision 4) > +Kinds are the focal point of this system. > +They provide an interface between the large-scale graph-generation process and the small-scale task-definition needs of different kinds of tasks. > +Each kind may implement task generation differently. > +Some kinds may generate task definitions entirely internally (for example, symbol-upload tasks are all alike, and very simple), while other kinds may do little more than parse a directory of YAML files. > + > +A `kind.yml` file contains data about the kind, as well as referring to a Python class implementing the kind in its ``implementation`` key. > +That implementation may rely on lots of code shared with other kinds, or contain a completely unique implementation of some functionality. > + > +The result is a nice segmentation of implementation so that the more esoteric in-tree projects can do their crazy stuff in an isolated kind without making the bread-and-butter build and test configuration more complicated. This should be wrapped at 80 characters. Use an empty line to create new paragraphs during formatting. ::: taskcluster/docs/taskgraph.rst:50 (Diff revision 4) > +(*) A kind can depend on itself, though. You can safely ignore that detail. > +Tasks can also be linked within a kind using explicit dependencies. There's probably a fancy way to do footnotes with Sphinx. But I'm too lazy to look it up. ::: taskcluster/docs/taskgraph.rst:68 (Diff revision 4) > +1. For all kinds, generate all tasks. The result is the "full task set" > + > +1. Create links between tasks using kind-specific mechanisms. The result is the "full task graph". You may want to double check the numbering works correctly here. It may start new numbers because of the empty line :/ ::: taskcluster/docs/taskgraph.rst:87 (Diff revision 4) > +------------- > + > +A number of mach subcommands are available aside from ``mach taskgraph decision`` to make this complex system more accesssible to those trying to understand or modify it. > +They allow you to run portions of the graph-generation process and output the results. > + > +* ``mach taskgraph tasks`` -- get the full task set A better way to do this in ReST is a definition list. e.g. ``mach taskgraph tasks`` get the full task set ``mach taskgraph full`` get the full task graph Just indent something 3 spaces on the line below and it is formatted all nice. ::: taskcluster/docs/taskgraph.rst:96 (Diff revision 4) > +* ``mach taskgraph optimized`` -- get the optimized task graph > +* ``mach taskgraph decision`` -- run the whole task-graph generation process (expects to be run in a decision task) > + > +Each of these commands taskes a ``--parameters`` option giving a file with parameters to guide the graph generation. > +The decision task helpfully produces such a file on every run, and that is generally the easiest way to get a parameter file. > +The parameter keys and values are described in :doc:`parameters`. Another likely link/ref issue. ::: taskcluster/mach_commands.py:56 (Diff revision 4) > + by dependencies: for example, a binary must be built before it is tested, > + and that build may further depend on various toolchains, libraries, etc. > + """ > + > + @SubCommand('taskgraph', 'python-tests', > + 'Run the taskgraph unit tests') Does this help string get rendered properly? I thought you needed to pass `description=msg` for this to work. The docstring of commands is also used when printing long form help. e.g. `mach help taskgraph python-tests` prints the docstring. `mach help taskgraph` prints the short description. ::: taskcluster/mach_commands.py:58 (Diff revision 4) > + """ > + > + @SubCommand('taskgraph', 'python-tests', > + 'Run the taskgraph unit tests') > + def taskgraph_python_tests(self, **options): > + import unittest Historical context: we have mozunit to format test output in a way that Treeherder can parse. If you plan on running this in automation (which you should), Treeherder may not detect failures properly. It will get the status code, but it won't parse out what failed and link to that failure. This could likely be deferred to a follow-up. Before: if you break tests before this is fixed, the sheriffs may invoke their wrath. ::: taskcluster/mach_commands.py:68 (Diff revision 4) > + sys.exit(1) > + > + @ShowTaskGraphSubCommand('taskgraph', 'tasks', > + "Show all tasks in the taskgraph") > + def taskgraph_tasks(self, **options): > + return self.show_taskgraph('full_task_set', False, options) show_taskgraph() takes 2 arguments yet you are passing 3 here and for all the commands. Are you sure this works? ::: taskcluster/mach_commands.py:90 (Diff revision 4) > + @SubCommand('taskgraph', 'decision', textwrap.dedent("""\ > + Decision task: generate a task graph and submit to TaskCluster. > + This is only meant to be called within decision tasks, and > + requires a great many arguments. Commands like `mach > + taskgraph optimized` are better suited to use on the command > + line, and can take the parameters file generated by a decision task. > + """)) You should use a one-liner for the description and put the verbose docs in a docstring. ::: taskcluster/mach_commands.py:146 (Diff revision 4) > + import taskgraph.target_tasks > + import taskgraph.generator > + > + parameters = taskgraph.parameters.load_parameters_file(options) > + > + optimization_finder = lambda keys: {} # XXX Ummm.

https://reviewboard.mozilla.org/r/50045/#review48901 Reviewed graph.py. ::: taskcluster/taskgraph/graph.py:21 (Diff revision 4) > + graph is not checked for cycles, and methods may hang or otherwise fail if > + given a cyclic graph. > + > + The `nodes` and `edges` attributes may be accessed in a read-only fashion. > + The `nodes` attribute is a set of node names, while `edges` is a set of > + `(left, right, name)` tuples representing an edge named `name` going from I could bikeshed this and suggest you use "from" and "to" to reinforce edge direction. I'll let you decide. Are "left" and "right" common in graph theory? ::: taskcluster/taskgraph/graph.py:28 (Diff revision 4) > + """ > + > + def __init__(self, nodes, edges): > + """ > + Create a graph. Nodes is a set of node names, while edges is a set of > + (start, end) pairs of node names. Both values are used by reference, Edges is a set of 3-tuples. ::: taskcluster/taskgraph/graph.py:53 (Diff revision 4) > + assert isinstance(nodes, set) > + assert nodes <= self.nodes > + > + # generate a new graph by expanding along edges until reaching a fixed > + # point > + new_nodes, new_edges = nodes, set() It scares me that `new_nodes` is assigned by reference. But the usage doesn't modify, so it is fine. ::: taskcluster/taskgraph/graph.py:71 (Diff revision 4) > + visited *after* any nodes it links to. > + > + Behavior is undefined (read: it will hang) if the graph contains a > + cycle. > + """ > + queue = collections.deque(self.nodes) I generally like throwing in a sorted() whenever I go from an unordered data structure to an ordered one so behavior is deterministic. That helps reproduce bugs and keeps things from being intermittent. I can guarantee if there were a bug in this code that only manifested on same graph generation runs you would tear your hair out :) ::: taskcluster/taskgraph/graph.py:83 (Diff revision 4) > + for n in links: > + queue.append(n) > + queue.append(node) collections.deque is really slow (at least compared to other Python data structures). I'm pretty sure it is implemented as a list under the hood, which means that popleft() requires allocating a new list. Therefore, testing `node in seen` before adding items that will just be ignored is probably an optimization worth taking. I believe this also means you can remove the `if node in seen` check above, since it will never occur. ::: taskcluster/taskgraph/graph.py:92 (Diff revision 4) > + def links_dict(self): > + """ > + Return a dictionary mapping each node to a set of its downstream > + nodes (omitting edge names) > + """ > + links = collections.defaultdict(lambda: set()) defaultdict(set) is equivalent. ::: taskcluster/taskgraph/graph.py:102 (Diff revision 4) > + def reverse_links_dict(self): > + """ > + Return a dictionary mapping each node to a set of its upstream > + nodes (omitting edge names) > + """ > + links = collections.defaultdict(lambda: set()) Ditto.

https://reviewboard.mozilla.org/r/50045/#review48911 And test_graph.py ::: taskcluster/taskgraph/test/test_graph.py:15 (Diff revision 4) > + > +from mozunit import main > + > +class TestGraph(unittest.TestCase): > + > + tree = Graph(set('abcdefg'), { Might want to throw in a quick comment that `set(str)` treats the string as an iterable and splits on each chacter. This initially tripped me up when reading this because 99% of the time `set(str)` is a bug :) ::: taskcluster/taskgraph/test/test_graph.py:102 (Diff revision 4) > + self.assert_postorder(self.diamonds.visit_postorder(), self.diamonds.nodes) > + > + def test_visit_postorder_multi_edges(self): > + "postorder visit of a graph with duplicate edges satisfies invariant" > + self.assert_postorder(self.multi_edges.visit_postorder(), self.multi_edges.nodes) > + Can you add a test for visit_postorder() with disjoint graphs?

https://reviewboard.mozilla.org/r/50045/#review48921 Looked at most of try_option_syntax. Then my eyes started glazing over. I'll do a more thorough review of try syntax parsing on the next iteration. Plus it looks like sfink looked at it, so I'm willing to rubber stamp if he's fine with it. ::: taskcluster/taskgraph/try_option_syntax.py:108 (Diff revision 4) > +} > + > +# We have a few platforms for which we want to do some "extra" builds, or at > +# least build-ish things. Sort of. Anyway, these other things are implemented > +# as different "platforms". > +RIDEALONG_BUILDS = { We should rename this to "hop ons" if you can come up with a subtle Arrested Development reference :) ::: taskcluster/taskgraph/try_option_syntax.py:155 (Diff revision 4) > + if try_idx is None: > + self.make_empty() I'd move the default assignments from `make_empty` into `__init__` and remove `make_empty` ::: taskcluster/taskgraph/try_option_syntax.py:162 (Diff revision 4) > + parser.add_argument('-p', '--platform', nargs='?', dest='platforms', const='all', default='all') > + parser.add_argument('-u', '--unittests', nargs='?', dest='unittests', const='all', default='all') When is the appropriate time to change the default from "execute everything" to "execute something sane" (like just linux64)? Given the capacity issues of Windows and OS X at the moment, I'd love to find ways to prevent people from overloading Try. (This can likely be deferred to a follow-up bug because it warrants its own conversation.) ::: taskcluster/taskgraph/try_option_syntax.py:244 (Diff revision 4) > + # Special case where tests is 'all' and must be expanded > + if tests[0]['test'] == 'all': > + results = [] > + all_entry = tests[0] > + for test in all_tests: > + entry = { 'test': test } Nit: cuddle braces ::: taskcluster/taskgraph/try_option_syntax.py:255 (Diff revision 4) > + Parse `testspec,testspec,..`, where each testspec is a test name > + optionally followed by a list of platforms or negated platforms in > + `[]`. This needs to be documented better. Even I don't know how testspecs work in Try syntax :/ ::: taskcluster/taskgraph/try_option_syntax.py:429 (Diff revision 4) > + if self.platforms is not None: > + if attr('build_platform') not in self.platforms: This is an anti-pattern in Python. It is preferable to use an empty list to denote falsy.

https://reviewboard.mozilla.org/r/50045/#review48927 Here's the next batch. My brain is starting to melt. I'm going to take a little break and try to finish this up later. There are definitely enough comments for another round or two of review. Unfortunately I haven't got to the meat of the patch: the core decision/graph building logic. Been slowly working my way from bottom to top. ::: taskcluster/taskgraph/create.py:29 (Diff revision 4) > + task_def = task.kind.get_task_definition(task, deps_by_name) > + task_def['taskGroupId'] = task_group_id > + task_def['dependencies'] = deps_by_name.values() > + task_def['requires'] = 'all-completed' > + > + _create_task(label_to_taskid[label], task_def) Having to make a single HTTP request for every task is itself inefficient. But even worse than that is having to establish a new TCP connection for each request. Please use a `requests.Session` instance so we reuse the TCP socket and issue HTTP requests faster. Bonus points if TC gains a batch task submit API. ::: taskcluster/taskgraph/create.py:35 (Diff revision 4) > + > +def _create_task(task_id, task_def): > + # create the task using 'http://taskcluster/queue', which is proxied to the queue service > + # with credentials appropriate to this job. > + print("Creating task {}".format(task_id)) > + res = requests.put('http://taskcluster/queue/v1/task/{}'.format(task_id), data=json.dumps(task_def)) http:// makes me sad. But I guess there is light magic involved. ::: taskcluster/taskgraph/create.py:38 (Diff revision 4) > + print res.json()['message'] > + except: > + print res.text Can this use a log? print() inside of generic library functions isn't a best practice. ::: taskcluster/taskgraph/decision.py:21 (Diff revision 4) > +import taskgraph.target_tasks > + > +ARTIFACTS_DIR = 'artifacts' > + > + > +def taskgraph_decision(log, options): This function needs a docstring given its importance. ::: taskcluster/taskgraph/decision.py:82 (Diff revision 4) > + }, 'writing artifact file `{filename}`') > + if not os.path.isdir(ARTIFACTS_DIR): > + os.mkdir(ARTIFACTS_DIR) > + path = os.path.join(ARTIFACTS_DIR, filename) > + if filename.endswith('.yml'): > + import yaml Import this at module level. Only mach commands should use the delay import foo (that's to make `mach` start faster). ::: taskcluster/taskgraph/generator.py:128 (Diff revision 4) > + impl_module, impl_object = impl.split(':') > + impl_class = __import__(impl_module) > + for a in impl_module.split('.')[1:]: > + impl_class = getattr(impl_class, a) > + for a in impl_object.split('.'): > + impl_class = getattr(impl_class, a) There /might/ be a helper function in pkg_resources or distutils that does this. I don't know off hand though. ::: taskcluster/taskgraph/kind/base.py:29 (Diff revision 4) > + def load_tasks(self, parameters): > + """ > + Get the set of tasks of this kind. > + > + The `parameters` give details on which to base the task generation. > + See `taskcluster/PARAMETERS.md` for details. Does `PARAMTERS.md` still exist? ::: taskcluster/taskgraph/kind/legacy.py:12 (Diff revision 4) > +from . import base > +from taskgraph.types import Task Feels weird that you're mixing absolute and relative imports here. Personally, I'm a fan of `from .types import Task` syntax because it still works if you rename packages. Plus with Python 2 without `from __future__ import absolute_import` it is unambiguous which module you are importing. Speaking of `absolute_import`, please use it everywhere to buffer against future Python 3 compatibility issues. ::: taskcluster/taskgraph/kind/legacy.py:50 (Diff revision 4) > +# actual taskIds at the very last minute, in get_task_definition > +TASKID_PLACEHOLDER = 'TaskLabel=={}' > + > +ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{}/artifacts/{}' > +DEFINE_TASK = 'queue:define-task:aws-provisioner-v1/{}' > +DEFAULT_TRY = 'try: -b do -p all -u all -t all' I hate this default. I'm going to file a bug to change it.

https://reviewboard.mozilla.org/r/50045/#review48987 I've made it through everything except LegacyKind:load_tasks and some of the unit tests. Everything looks pretty good architecturally. ::: taskcluster/taskgraph/kind/legacy.py:100 (Diff revision 4) > + self.log(logging.DEBUG, 'vcs-info', {}, > + '%d commits influencing task scheduling:\n' % len(vcs_info.changesets)) > + for c in vcs_info.changesets: > + self.log(logging.DEBUG, 'vcs-relevant-commit', { > + 'cset': c['node'][0:12], > + 'desc': c['desc'].splitlines()[0].encode('ascii', 'ignore'), ascii?! Can't we use utf-8 with "replace"? ::: taskcluster/taskgraph/kind/legacy.py:238 (Diff revision 4) > + routes_transform.decorate_task_json_routes(build_task['task'], > + json_routes, > + build_parameters) > + > + # Ensure each build graph is valid after construction. > + taskcluster_graph.build_task.validate(build_task) This is about where my brain starts to turn to mush. This function is way too long and way under-documented. In your defense, this code is largely copied, so I can't complain too much. Anything you can do to increase the readability of this would be appreciated. ::: taskcluster/taskgraph/parameters.py:33 (Diff revision 4) > + filename = options['parameters'] > + if not filename: > + return Parameters() > + with open(filename) as f: > + if filename.endswith('.yml'): > + import yaml Move import to module level. ::: taskcluster/taskgraph/parameters.py:38 (Diff revision 4) > + print("Parameters file `{}` is not JSON or YAML".format(filename)) > + sys.exit(1) This should be an exception. ::: taskcluster/taskgraph/parameters.py:59 (Diff revision 4) > + for n in option_names: > + if options[n]: > + parameters[n] = options[n] parameters = {n: options[n] for n in option_names if options[n]} ::: taskcluster/taskgraph/target_tasks.py:31 (Diff revision 4) > + > +@_target_task('try_option_syntax') > +def target_tasks_try_option_syntax(full_task_graph, parameters): > + """Generate a list of target tasks based on try syntax in > + parameters['message'] and, for context, the full task graph.""" > + from taskgraph.try_option_syntax import TryOptionSyntax Move import to module level. ::: taskcluster/taskgraph/types.py:24 (Diff revision 4) > + if not attributes: > + attributes = {} > + if not task: > + task = {} attributes = attributes or {} task = task or {} ::: taskcluster/taskgraph/types.py:37 (Diff revision 4) > + for item in attributes.iteritems(): > + assert all(isinstance(x, basestring) for x in item), \ Ahh, this is where you validate attributes are strings! It's a bit weird to use `iteritems()` and then iterate over its return. It works, but it isn't a common Python pattern. You should make this raise TypeError or ValueError instead of the assert.

> ::: taskcluster/docs/attributes.rst:43 > (Diff revision 4) > > + * ``debug`` > > + * ``opt`` > > What about PGO? > > What about e.g. static analysis builds? Are these different build platforms? > Different build types? That is ill-defined right now (according to try syntax, they are platforms (-p), but not build types (-b)). This patch is not going to achieve world peace :) > While we're here, I'm trying to add job metrics to Perfherder in bug > 1271077. One of the things that's tripping me up is figuring out what "key" > to insert the data in under Perfherder. From mozharness, it isn't exactly > trivial to come up with a proper key. In buildbot, the key is the builder > name. However, those names are really long and wonky. It would be awesome if > TC had a way to generate a deterministic "task identifier" constructed from > the platform, type, flavor, chunk, etc. Use the task label (noting that tasks from the legacy kind have random task labels, but tasks from other kinds will have stable labels). > It feels weird that everything is a string despite using YAML, which can > preserve types. I'm generally a fan of strong typing where it can prevent > bugs. > > I haven't looked at the code yet: hopefully the YAML loading code raises an > error if it doesn't encounter a string in YAML. I'm not sure why you think this is YAML -- it's a Python data structure. I chose to make (and assert, as you see elsewhere in this patch) all attributes are strings for consistency of expectations. If I allow multiple types, then we're susceptible to subtle bugs where kindA makes integer chunks and kindB makes string chunks, so that regexp you tested so well against kindB fails for try pushes including kindA. Adding and validating a schema is possible, and the simplest way to do that is to just assert everything is a string, as chunks are the only numeric quantity I can think of appearing here (and they are always treated as disjoint sets, not ranges). > Does this ref work? I thought you needed to do e.g. > :ref:`taskcluster_attributes` (which requires a `.. taskcluster_attributes:` > at the top of attributes.rst to define that "anchor." http://www.sphinx-doc.org/en/stable/markup/inline.html#role-doc > ::: taskcluster/taskgraph/try_option_syntax.py:162 > (Diff revision 4) > > + parser.add_argument('-p', '--platform', nargs='?', dest='platforms', const='all', default='all') > > + parser.add_argument('-u', '--unittests', nargs='?', dest='unittests', const='all', default='all') > > When is the appropriate time Not now. Again, this bug does not aim to achieve world peace. > ::: taskcluster/taskgraph/try_option_syntax.py:429 > (Diff revision 4) > > + if self.platforms is not None: > > + if attr('build_platform') not in self.platforms: > > This is an anti-pattern in Python. It is preferable to use an empty list to > denote falsy. I think you've misunderstood the code -- None means something different from [] here. > ascii?! Can't we use utf-8 with "replace"? That was bug 1252440 - feel free to re-open. > ::: taskcluster/taskgraph/kind/legacy.py:238 > (Diff revision 4) > > + routes_transform.decorate_task_json_routes(build_task['task'], > > + json_routes, > > + build_parameters) > > + > > + # Ensure each build graph is valid after construction. > > + taskcluster_graph.build_task.validate(build_task) > > This is about where my brain starts to turn to mush. This function is way > too long and way under-documented. In your defense, this code is largely > copied, so I can't complain too much. Anything you can do to increase the > readability of this would be appreciated. I have already promised to delete it: the ultimate in readability improvements! However, I can't do that until I land it :) > ::: taskcluster/taskgraph/parameters.py:38 > (Diff revision 4) > > + print("Parameters file `{}` is not JSON or YAML".format(filename)) > > + sys.exit(1) > > This should be an exception. Exceptions generate a mach message that says "this should not happen", so I've assumed that I should handle all user errors (the kind that should happen) in some other fashion. Are you suggesting I raise an exception here and catch it where this function is called, then print/sys.exit?

(In reply to Gregory Szorc [:gps] from comment #65) > ::: taskcluster/taskgraph/graph.py:21 > (Diff revision 4) > > + graph is not checked for cycles, and methods may hang or otherwise fail if > > + given a cyclic graph. > > + > > + The `nodes` and `edges` attributes may be accessed in a read-only fashion. > > + The `nodes` attribute is a set of node names, while `edges` is a set of > > + `(left, right, name)` tuples representing an edge named `name` going from > > I could bikeshed this and suggest you use "from" and "to" to reinforce edge > direction. I'll let you decide. Are "left" and "right" common in graph > theory? "from" is a keyword, so that's out. `left` and `right` will be fine :)

https://docs.python.org/2/library/collections.html#collections.deque --- Deques support thread-safe, memory efficient appends and pops from either side of the deque with approximately the same O(1) performance in either direction. Though list objects support similar operations, they are optimized for fast fixed-length operations and incur O(n) memory movement costs for pop(0) and insert(0, v) operations which change both the size and position of the underlying data representation. --- And indeed, deques are about 30% faster than lists for popping on the left and pushing on the right.

Comment on attachment 8748839 [details] MozReview Request: Bug 1258497: allow mozunit to pass along kwargs; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50039/diff/3-4/

Comment on attachment 8748840 [details] MozReview Request: Bug 1258497: Factor out some common code; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50041/diff/3-4/

Comment on attachment 8748842 [details] MozReview Request: Bug 1258497: Implement a new taskgraph generation system; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50045/diff/4-5/

Comment on attachment 8748841 [details] MozReview Request: Bug 1258497: use `taskgraph decision` in decision task; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50043/diff/3-4/

Comment on attachment 8748842 [details] MozReview Request: Bug 1258497: Implement a new taskgraph generation system; r?gps https://reviewboard.mozilla.org/r/50045/#review49145 ::: taskcluster/mach_commands.py:69 (Diff revision 5) > + sys.exit(1) > + > + @ShowTaskGraphSubCommand('taskgraph', 'tasks', > + description="Show all tasks in the taskgraph") > + def taskgraph_tasks(self, **options): > + return self.show_taskgraph('full_task_set', options) OK. The comments about something should be raising an exception reminded me that mach @Command handlers will print the "unexpected error" message for uncaught exceptions. It is considered a best practice to add a try..except Exception block in @Command methods. Have them print the exception and return 1. Given the number of commands that get this wrong (I even forgot about it!), we should probably provide a feature in mach itself to handle exceptions nicer. But that's for another bug.

Review commit: https://reviewboard.mozilla.org/r/52864/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52864/

Comment on attachment 8748841 [details] MozReview Request: Bug 1258497: use `taskgraph decision` in decision task; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50043/diff/4-5/

Comment on attachment 8748842 [details] MozReview Request: Bug 1258497: Implement a new taskgraph generation system; r?gps https://reviewboard.mozilla.org/r/50045/#review49772 This is a massive patch. I'm sure there are bugs. There are plenty of things we could bikeshed. But, perfect is the enemy of done. I've looked at this several times and like the overall approach. I'm pretty confident bugs in this code will surface pretty quickly in automation. It is a Monday, which means we'll have the whole week to triage fires if they surface. So r+ from me. ::: taskcluster/docs/parameters.rst:21 (Diff revision 5) > + The repository from which to do an initial clone, utilizing any available > + caching. I believe these all need 3 spaces of indent. ::: taskcluster/docs/taskgraph.rst:98 (Diff revision 5) > +``mach taskgraph tasks`` > + Get the full task set 3 space indent. ::: taskcluster/taskgraph/create.py:13 (Diff revision 5) > +import json > +import collections > + > +from slugid import nice as slugid > + > +session = requests.Session() Global variables are evil. Move this to create_tasks() and pass it to _create_task(). ::: taskcluster/taskgraph/generator.py:21 (Diff revision 5) > + generation. The task is generated from all of the kinds defined in > + subdirectories of the generator's root directory. > + > + Access to the results of this generation, as well as intermediate values at > + various phases of generation, is available via properties. This encourages > + the provision of all generation inputs at instance construction time. Trailing whitespace. ::: taskcluster/taskgraph/parameters.py:14 (Diff revision 5) > +import json > +import sys > +import yaml > + > +class Parameters(dict): > + """An immutable dictionary with nicer KeyError messages on failure""" FWIW, there are some nice data structures (including a ReadOnlyDict) in python/mozbuild/mozbuild/util.py.

Comment on attachment 8748841 [details] MozReview Request: Bug 1258497: use `taskgraph decision` in decision task; r=gps https://reviewboard.mozilla.org/r/50043/#review49776

Comment on attachment 8752956 [details] MozReview Request: Bug 1258497: wrap mach Commands in try/except; r=gps https://reviewboard.mozilla.org/r/52864/#review49778

Comment on attachment 8748842 [details] MozReview Request: Bug 1258497: Implement a new taskgraph generation system; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50045/diff/5-6/

Comment on attachment 8748841 [details] MozReview Request: Bug 1258497: use `taskgraph decision` in decision task; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50043/diff/5-6/

Comment on attachment 8752956 [details] MozReview Request: Bug 1258497: wrap mach Commands in try/except; r=gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52864/diff/1-2/

https://hg.mozilla.org/integration/mozilla-inbound/rev/2148df37953cb6dddf9f9a0ac95a38734b49e415 Bug 1258497: allow mozunit to pass along kwargs; r=gps https://hg.mozilla.org/integration/mozilla-inbound/rev/997ae5a6386621b88513bf5322cd4c82a63862bb Bug 1258497: Factor out some common code; r=gps https://hg.mozilla.org/integration/mozilla-inbound/rev/c3e24e94ab5148ad92a3f137d6cab9d8bebdd6a7 Bug 1258497: Implement a new taskgraph generation system; r=gps https://hg.mozilla.org/integration/mozilla-inbound/rev/c17e66a20cb3e167420df885f94c6c010d1f6089 Bug 1258497: use `taskgraph decision` in decision task; r=gps https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe18230f273f0e0780048056dc50dec026a18c5 Bug 1258497: wrap mach Commands in try/except; r=gps

..and on inbound, that ran the "old" decision task, as expected. that gives me a little breathing room to bake this in try, and to work on mozilla-taskcluster integration.

https://hg.mozilla.org/mozilla-central/rev/2148df37953c https://hg.mozilla.org/mozilla-central/rev/997ae5a63866 https://hg.mozilla.org/mozilla-central/rev/c3e24e94ab51 https://hg.mozilla.org/mozilla-central/rev/c17e66a20cb3 https://hg.mozilla.org/mozilla-central/rev/7fe18230f273