Closed Bug 1302802 Opened 8 years ago Closed 7 years ago

Ensure that task graphs do not contain duplicate Treeherder symbols

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1323633

People

(Reporter: dustin, Assigned: hammad13060, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Treeherder decides what symbol to print, and where, based on
  task.extra.treeherder.machine.platform
  task.extra.treeherder.groupSymbol
  task.extra.treeherder.symbol

We should never generate two tasks with the same symbol.  The decision task should verify this and fail if we do.

Bonus points for implementing a fairly generic way to plug in graph verification functions.
Assignee: nobody → hammad13060
Hey Dustin, by "we should never generate two tasks with same symbol". Do you mean, there should not be more than one task with same symbol, for specific platform and groupSymbol ? or I need to take care of only symbol value irrespective of platform and groupSymbol ?
Yes, the tuple (machine.platform, groupSymbol, symbol) should be unique.
Comment on attachment 8816895 [details]
Bug 1302802 - Ensure that task graphs do not contain duplicate Treeherder symbols;

Hey Dustin, please have a look at current design. Commit is failing on try because of Bug 1289779. I will also resolve the latter.

P.S. Do you find current design generic enough ?
Comment on attachment 8816895 [details]
Bug 1302802 - Ensure that task graphs do not contain duplicate Treeherder symbols;

https://reviewboard.mozilla.org/r/97410/#review98362

I like this approach!

::: taskcluster/taskgraph/generator.py:15
(Diff revision 1)
>  from .util.verifydoc import verify_docs
> +from .util import verifytaskgraph as VerifyTaskGraph

Could you combine both of these into a single module named "verify"?

::: taskcluster/taskgraph/generator.py:16
(Diff revision 1)
>  from .graph import Graph
>  from .taskgraph import TaskGraph
>  from .optimize import optimize_task_graph
>  from .util.python_path import find_object
>  from .util.verifydoc import verify_docs
> +from .util import verifytaskgraph as VerifyTaskGraph

why change the capitalization here?

I think you could just use

    from .util.verifytaskgraph import verify_task_graph_symbol

::: taskcluster/taskgraph/taskgraph.py:49
(Diff revision 1)
>              if task.task_id:
>                  task_json['task_id'] = task.task_id
>              tasks[key] = task_json
>          return tasks
>  
> +    def for_each_task(self, f):

I like this!

The scratchpad is a neat idea.  A more common approach is to allow extra arguments to be passed along with the function to call -- something like

    tg.for_each_task(myfunc, {})
  
So

    def for_each_task(self, f, *args, **kwargs):
        ...
        f(task, task_label, self, *args, **kwargs)

It might make sense to omit the label argument, since it is accessible as `task.label`.

Please add a docstring, too!

::: taskcluster/taskgraph/util/verifytaskgraph.py:14
(Diff revision 1)
> +            try:
> +                platform = treeherder["machine"]["platform"]
> +            except KeyError:
> +                platform = None

A much simpler way to write this:

    platform = treeherder.get('machine', {}).get('platform')
    
similarly for group_symbol and symbol.
Attachment #8816895 - Flags: review?(dustin)
Comment on attachment 8816895 [details]
Bug 1302802 - Ensure that task graphs do not contain duplicate Treeherder symbols;

https://reviewboard.mozilla.org/r/97410/#review98526
Comment on attachment 8816895 [details]
Bug 1302802 - Ensure that task graphs do not contain duplicate Treeherder symbols;

https://reviewboard.mozilla.org/r/97410/#review98590

This looks great!  One little Python-ism to fix, and I'll trigger a test in try just to be sure, and then we can land this!

::: taskcluster/taskgraph/util/verify.py:59
(Diff revision 3)
> +
> +            platform = treeherder.get('machine', {}).get('platform')
> +            group_symbol = treeherder.get('groupSymbol')
> +            symbol = treeherder.get('symbol')
> +
> +            key = tuple([platform, group_symbol, symbol])

key = (platform, group_symbol, symbol)

would be simpler!
Attachment #8816895 - Flags: review?(dustin) → review+
Comment on attachment 8816895 [details]
Bug 1302802 - Ensure that task graphs do not contain duplicate Treeherder symbols;

https://reviewboard.mozilla.org/r/97410/#review98616

Exception: conflict between `build-docker-image-desktop-build`:`build-docker-image-android-gradle-build` for values `(None, None, 'I')`

so I think we'll need to land the image-naming patch first!
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6034929c3e4a
Ensure that task graphs do not contain duplicate Treeherder symbols; r=dustin
Dustin it seems that all build-images have same TreeHerder symbol tc(B). I think we can rename them to resolve the issue.
Indeed -- I'm not sure why there are two tc(B) for linux32, though -- there should only be one build.

The `jq` tool (https://stedolan.github.io/jq/) can be useful for analyzing the JSON of the full taskgraph.
No, there should be two builds: build-linux/opt and build-linux/debug. If you want uniqueness with machine.platform, which is linux32 for both of those, then you need to throw whatever has opt or debug into the mix too.
(In reply to Phil Ringnalda (:philor) from comment #16)
> No, there should be two builds: build-linux/opt and build-linux/debug. If
> you want uniqueness with machine.platform, which is linux32 for both of
> those, then you need to throw whatever has opt or debug into the mix too.

Can we do the following:
- for linux32 opt we can make tc(l32o) as taskgroup
- for linux32 debug we can make tc(l32d) as taskgroup

P.S we can do the same for all the builds
The opt and debug builds are displayed on two different rows on treeherder: it is not a bug that they both have the same symbol, and the solution is not to change how they are displayed.

Four possibilities:

* comment 0 is wrong, and where and how they are displayed on treeherder is based on four things, machine.platform, foo.bar.baz (whatever it is that has the value 'opt' or 'debug'), groupSymbol, and symbol, and the uniqueness test needs to be over those four things instead of just three

* comment 0 is correct about what is supposed to happen, but at least for linux32 the machine.platform value is buggy and fails to include the opt/debug that it should, but either intentionally or accidentally treeherder works around that and knows which is opt and which is debug anyway

* somehow, when treeherder sees it machine.platform for those two builds is 'linux32/opt' and 'linux32/debug', but when you see it it is still 'linux32' for both

* comment 0 is wrong, and treeherder bases what row to display jobs on on something completely different than machine.platform, something which actually does contain both that it is linux32 and whether it is opt or debug
(In reply to Phil Ringnalda (:philor) from comment #18)
> * comment 0 is wrong, and where and how they are displayed on treeherder is
> based on four things, machine.platform, foo.bar.baz (whatever it is that has
> the value 'opt' or 'debug'), groupSymbol, and symbol, and the uniqueness
> test needs to be over those four things instead of just three

This is correct.

        "treeherder": {
          "collection": {
            "opt": true
          },
          "groupName": "Executed by TaskCluster",
          "groupSymbol": "tc",
          "jobKind": "build",
          "machine": {
            "platform": "android-4-0-armv7-api15"
          },
          "symbol": "Deps",
          "tier": 2
        },

so we need collection (I have no idea why it's a dictionary; just take sorted(collection.keys())), groupSymbol, symbol, and machine.platform.  Sorry that I forgot about the collection initially!

Thanks Philor :)
Depends on: 1323633
Blocks: 1323633
No longer depends on: 1323633
Continued in bug 1323633
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Blocks: 1325398
Blocks: 1326462
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.