Verify taskgraph implementations against documentation

RESOLVED FIXED in mozilla53

Status

Taskcluster
Task Configuration
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: dustin, Assigned: HAMMAD AKHTAR, Mentored)

Tracking

unspecified
mozilla53

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
The docs in taskcluster/docs contain a few lists of things -- attributes, parameters, kinds, caches, run-using implementations.

It would be great if something, either as part of the `mach taskgraph python-tests` or as part of the decision task, would ensure that those stay in sync with the code.

Some simple regexps over the documentation files would do the trick -- I don't think that trying to parse the files with docutils is a good use of time.
(Reporter)

Updated

9 months ago
Assignee: nobody → hammad13060
Comment hidden (mozreview-request)
(Assignee)

Comment 2

9 months ago
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

I am trying to develop generic method for doc verification against implementation. 
Please provide your feedback.
P.S patch is incomplete right now
Comment hidden (mozreview-request)
(Reporter)

Comment 4

9 months ago
mozreview-review
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review96026

::: taskcluster/taskgraph/generator.py:240
(Diff revision 2)
> +    @VerifyDoc.verify_doc_file("documentation and implementation for 'kinds' do not match", os.getcwd() + "/taskcluster/docs/")
> +    def verify_kinds(self, kinds):
> +        return {
> +            "kinds.rst": [kind for kind in kinds]
> +        }

I don't see a great advantage to using a decorator here.  Why not just

    def verify_kinds(self, kinds):
        verify_docs(
            filename='kinds.rst',
            identifiers=kinds.keys(),
            appearing_as='heading')

::: taskcluster/taskgraph/generator.py:256
(Diff revision 2)
> +            for attribute in task.attributes:
> +                attribute_set.add(attribute)

`attribute_set.update(task.attributes)` will be faster

::: taskcluster/taskgraph/generator.py:265
(Diff revision 2)
> +        for kind_label, kind in kinds.iteritems():
> +            if "jobs" in kind.config:
> +                for job_label, job in kind.config["jobs"].iteritems():
> +                    if "run" in job and "using" in job["run"]:
> +                        key = job["run"]["using"]
> +                        job_set.add(key)

This is reaching a little too deeply into the kind definitions.  In particular, they might have `jobs-from` instead of `jobs`.  Or some kinds might use completely different config.

However, `taskgraph.transforms.job.registry` has exactly the list of of run-using's that you need.

::: taskcluster/taskgraph/util/verifydoc.py:6
(Diff revision 2)
> +import re
> +import os
> +
> +class VerifyDoc(object):
> +
> +	@classmethod

A class with a single class method is just about always better implemented as a simple function.
Attachment #8814487 - Flags: review?(dustin)
(Reporter)

Comment 5

9 months ago
I really like this comprehensive approach -- keep it up!
(Assignee)

Comment 6

9 months ago
Hey Dustin thanks for the feedback :)
Can you elaborate on `taskgraph.transforms.job.registry`. Are you referring to registry dictionary in `transforms/__init__.py` ?
(Reporter)

Comment 7

9 months ago
That's the one!
Comment hidden (mozreview-request)
(Assignee)

Comment 9

9 months ago
Hey Dustin, can you shed some light on caches ?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 14

9 months ago
mozreview-review
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review96888

I love the design!  This is great!

Just a few (ok, a lot) Python style points.  Some of these are probably applicable to any kind of Python, while others are specific to how we write Python in the Gecko source.

::: taskcluster/taskgraph/util/verifydoc.py:1
(Diff revision 7)
> +import re

This will need a copyright header like the other Python files (just copy/paste it)

Also, please use four-space indents.

::: taskcluster/taskgraph/util/verifydoc.py:4
(Diff revision 7)
> +import re
> +import os
> +
> +base_path = os.getcwd() + "/taskcluster/docs/"

best to use `os.path.join` here

::: taskcluster/taskgraph/util/verifydoc.py:8
(Diff revision 7)
> +
> +base_path = os.getcwd() + "/taskcluster/docs/"
> +
> +
> +def verify_docs(filename, identifiers, appearing_as):
> +	fileObject = open(base_path + filename)

In in-tree Python, we prefer `with open`:

    with open(os.path.join(base_path, filename)) as f:
        doctext = f.read()

::: taskcluster/taskgraph/util/verifydoc.py:10
(Diff revision 7)
> +	if appearing_as == "inline-literal":
> +		expression_list = ["``" + identifier + "``" for identifier in identifiers]
> +	elif appearing_as == "heading":
> +		expression_list = [identifier + "\n[-+\n*]+|[.+\n*]+" for identifier in identifiers]

Nicely done!

Please add an `else` branch here that raises an exception, to avoid typos like `"haeding"`.

::: taskcluster/taskgraph/util/verifydoc.py:15
(Diff revision 7)
> +	if appearing_as == "inline-literal":
> +		expression_list = ["``" + identifier + "``" for identifier in identifiers]
> +	elif appearing_as == "heading":
> +		expression_list = [identifier + "\n[-+\n*]+|[.+\n*]+" for identifier in identifiers]
> +
> +	for i in range(0, len(expression_list)):

`for expression, identifier in zip(expression_list, identifiers)`
Attachment #8814487 - Flags: review?(dustin)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 19

9 months ago
mozreview-review
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review97232

At a glance, this is looking better, but I see more review requests coming .. let me know when you're ready for me to do the final review!
Attachment #8814487 - Flags: review?(dustin)
(Assignee)

Comment 20

9 months ago
(In reply to Dustin J. Mitchell [:dustin] from comment #19)
> Comment on attachment 8814487 [details]
> Bug 1302800 - Verify taskgraph implementations against documentation;
> 
> https://reviewboard.mozilla.org/r/95702/#review97232
> 
> At a glance, this is looking better, but I see more review requests coming
> .. let me know when you're ready for me to do the final review!

I am ready for final review :)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

9 months ago
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

I am ready for final review :)
(Reporter)

Comment 23

9 months ago
mozreview-review
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review97638

::: taskcluster/docs/transforms.rst:154
(Diff revision 12)
> +  ``hazard``
> +  ``mach``
> +  ``mozharness``
> +  ``run-task``
> +  ``spidermonkey`` or ``spidermonkey-package`` or ``spidermonkey-mozjs-crate``
> +  ``toolchain-script``

I suspect this will render as a paragraph in the final output?  We'll probably want to make it a list.
Attachment #8814487 - Flags: review?(dustin) → review+

Comment 24

9 months ago
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/632a2fddbc63
Verify taskgraph implementations against documentation; r=dustin

Comment 25

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/632a2fddbc63
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Soooo, how do I run this test locally?

At first glance it looks to be part of the decision task alone, but if thats true, I think we should back out because a test that can break the entire build that can only be run in production (including try) isn't ideal for local iteration. (While the rest of the mach taskgraph stuff can indeed be run locally)

If it should be running as part of `./mach taskgraph full` it doesn't seem to be failing for me locally when I'm missing docs for some of the kinds on the `date` branch after merging this in...
Flags: needinfo?(hammad13060)

Comment 27

9 months ago
mozreview-review
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review97700

::: taskcluster/taskgraph/util/verifydoc.py:9
(Diff revision 12)
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import re
> +import os
> +
> +base_path = os.path.join(os.getcwd(), "taskcluster/docs/")

Shouldn't this be using __file__ in some way, rather than relying on a known cwd that just-happens-to-match the topsrcdir

Comment 28

9 months ago
mozreview-review
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review97704

::: taskcluster/taskgraph/util/verifydoc.py:18
(Diff revision 12)
> +    with open(os.path.join(base_path, filename)) as fileObject:
> +        doctext = "".join(fileObject.readlines())
> +        if appearing_as == "inline-literal":
> +            expression_list = ["``" + identifier + "``" for identifier in identifiers]
> +        elif appearing_as == "heading":
> +            expression_list = [identifier + "\n[-+\n*]+|[.+\n*]+" for identifier in identifiers]

Ah-Ha, found my issue with some debug logging...

 0:00.20 Testing match for expression u'balrog-l10n(?:\\n[-+\\n*]+|[\\.+\\n*]+)' using identifier u'balrog-l10n'
Traceback (most recent call last):
  File "/home/callek/mozilla/hg/mozilla-central/taskcluster/mach_commands.py", line 231, in show_taskgraph

That was after modifying this expression, the | here was effectively causing it to match either 'balrog-l10n\n[-+\n*]+' OR '[.+\n*]+'  I'll get a followup bug on file and a patch, unless you beat me to it.

Updated

9 months ago
Blocks: 1322193
Filed Bug 1322193  for the followup work, and assigned to Hammad for now.
Flags: needinfo?(hammad13060)
You need to log in before you can comment on or make changes to this bug.