1795921 - always_target tasks can no longer be optimized out on try

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, defect)

Description

[Julien Cristau [:jcristau]]

Since https://hg.mozilla.org/mozilla-unified/rev/b7380df3b9a8ca43204527dfe9f851ab39b00d3c, pushes to try (with try_task_config) run all tasks that set always_target, whereas previously they only ran when the relevant files changed.

As I understand it, this is because:

• try_task_config sets the optimize_target_tasks parameter to False (https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/taskcluster/gecko_taskgraph/decision.py#495) to prevent optimizing out tasks the user explicitly requested
• when optimize_target_tasks is false, when generating the graph we add the target task set to do_not_optimize (https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/taskcluster/gecko_taskgraph/generator.py#374)
• tasks in do_not_optimize run can't be optimized out, including using their when.files-changed setting (https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/third_party/python/taskcluster_taskgraph/taskgraph/optimize/base.py#196-199)
• since the patch referenced above, the target task set contains not only the tasks that the user explicitly requested, but also those with always_target enabled.

Comment 1

[Andrew Halberstadt [:ahal]]

The easiest fix would be to backout https://hg.mozilla.org/mozilla-unified/rev/b7380df3b9a8ca43204527dfe9f851ab39b00d3c (and discard https://github.com/taskcluster/taskgraph/pull/134). BUT always-target is a massive hack imo which adds a lot of complexity. It might be worth trying to come up with an alternative solution.

We could always backout in the short term for now if this is adding too many extra tasks to try. I think Thunderbird will also need a backout if we do.

Comment 2

[Andrew Halberstadt [:ahal]]

Hm, the intent of always-target was essentially to run cheap tasks like linters even when they were not requested on try. These days, the code-review task already depends on both these tasks and python-test tasks. What if we remove always-target from everything except the code-review task? That way the actual lint and python-test tasks would be in the target_task_graph instead of the target_task_set, and would still be candidates for optimization.

Comment 3

[Julien Cristau [:jcristau]]

Marco, do you think running the code-review task on all try pushes (vs just those from reviewbot) would be ok? Or would the extra pulse messages confuse things elsewhere?

Comment 4

[Andrew Halberstadt [:ahal]]

If it's not ok, we could create a new task similar to the code-review one except that doesn't do anything.

Comment 5

[Marco Castelluccio [:marco]]

We were thinking of collecting issues from autoland for before/after analysis, but currently the code review hook can be triggered in two different ways according to whether it's an autoland revision or a try revision.

For autoland, it's triggered when a task group finishes (https://github.com/mozilla/code-review/blob/1c7cf549925ed43136b8e7489f5583ba7494f89b/events/code_review_events/workflow.py#L381), setting AUTOLAND_TASK_GROUP_ID in the env https://github.com/mozilla/code-review/blob/1c7cf549925ed43136b8e7489f5583ba7494f89b/events/code_review_events/workflow.py#L335.
For try, it's triggered thanks to the project.relman.codereview.v1.try_ending pulse message, sent by the code-review task (https://searchfox.org/mozilla-central/rev/a64647a2125cf3d334451051491fef6772e8eb57/taskcluster/ci/code-review/kind.yml#31), because it has a binding https://hg.mozilla.org/ci/ci-configuration/file/e0992441e67d6855fe0efb4a3db224ba8b9fbd8b/hooks.yml#l166.

The bot then knows whether it was triggered from try or from autoland by looking at the env: https://github.com/mozilla/code-review/blob/1c7cf549925ed43136b8e7489f5583ba7494f89b/bot/code_review_bot/config.py#L56.

So in theory it would be ok to run the code-review task on autoland too, but we would need to make some adjustments to the hook and the bot so that we can differentiate between triggers from try and triggers from autoland.

Comment 6

[Marco Castelluccio [:marco]]

Sorry I misunderstood, the question was about always running on try.

At the moment, it would be a problem for the bot as the hook would be triggered and the bot would fail because it won't be able to load a revision, https://github.com/mozilla/code-review/blob/1c7cf549925ed43136b8e7489f5583ba7494f89b/bot/code_review_bot/cli.py#L142, https://github.com/mozilla/code-review/blob/1c7cf549925ed43136b8e7489f5583ba7494f89b/bot/code_review_bot/revisions.py#L144 (phabricator_diff is set as a parameter when pushing to try: https://searchfox.org/mozilla-central/rev/a64647a2125cf3d334451051491fef6772e8eb57/taskcluster/docs/parameters.rst#246).

Comment 7

[Andrew Halberstadt [:ahal]]

Ok, no problem.. we should be able to add a new task that also uses the code_review attribute to set dependencies. It might be a bit strange to have two of them running on reviewbot pushes, but not a big deal.

Comment 8

[Marco Castelluccio [:marco]]

Another option could be to send the pulse message only when phabricator_diff is set, but I don't know if that's feasible.
Or, yet another option, could be to modify the bot to ignore such pushes where phabricator_diff is not set.

Comment 9

[Andrew Halberstadt [:ahal]]

I'm going to back out the patch in bug 1759030 and then re-implement it using the task idea.

Comment 10

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1795921 - Back out changeset b7380df3b9a8, r?#releng-reviewers!

Comment 11

[Andrew Halberstadt [:ahal]]

Attachment: WIP: Bug 1795921 - Move 'always-target' implementation to a task

Essentially, there will be an always-target task that depends on
all tasks with the always-target attribute. The try target_task
methods will then explicitly add this task, thus causing all of its
dependencies to run even though they weren't explicitly scheduled.

Depends on D160411

Comment 12

[Andrew Halberstadt [:ahal]]

Hm, the intent of always-target was essentially to run cheap tasks like linters even when they were not requested on try. These days, the code-review task already depends on both these tasks and python-test tasks. What if we remove always-target from everything except the code-review task? That way the actual lint and python-test tasks would be in the target_task_graph instead of the target_task_set, and would still be candidates for optimization.

After writing a patch and some careful debugging, I don't think this approach is going to work after all. The code-review task uses soft-dependencies to depend on tasks that are remaining after optimization. The problem, is that soft-dependencies won't actually pull tasks into the target_graph. They need to be there already. This works for code-review because the tasks are there because of always_target. But if I replace always_target with a task, then there will be nothing that pulls them into the target graph..

Conversely, if I use dependencies instead of soft-dependencies, then they also wouldn't be candidates for optimization due to having a dependent task (and if-dependencies won't work either).

I have a sinking feeling that when I initially implemented the always-target feature, I had all this context paged into my mind.

Comment 13

[Andrew Halberstadt [:ahal]]

I think I'm leaning towards backing out https://hg.mozilla.org/mozilla-unified/rev/b7380df3b9a8ca43204527dfe9f851ab39b00d3c and going back to the initial always-target implementation.

Though instead of gating it on parameters["tasks_for"] == "hg-push", I think we should create a new enable_always_target parameter that defaults to False and is only set to True on try. Then, we can add a run-on-projects: [all] to any task that uses it. This way the always-target feature will only apply to try pushes, and we'll use the standard project filter for everything else.

Comment 14

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1795921 - [taskgraph] Sort output of label formatter, r?#releng-reviewers!

Leaving them unsorted is not only hard to read, but messes up --diff when
using labels and the optimized graph.

Depends on D160411

Comment 15

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1795921 - [taskgraph] Fix erroneous 'tasks_for: actions' in test parameter files, r?#releng-reviewers!

The real tasks_for is called action.

Depends on D160534

Comment 16

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1795921 - [taskgraph] Use an 'enable_always_target' param to control when 'always_target' should apply, r?#releng-reviewers!

This is a bit more general purpose and flexible than hardcoding hg-push here.
It is also something that can be upstreamed to standalone taskgraph.

Depends on D160535

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80eac7743f74
Back out changeset b7380df3b9a8, r=releng-reviewers,gbrown
https://hg.mozilla.org/integration/autoland/rev/3df4832d21a0
[taskgraph] Sort output of label formatter, r=releng-reviewers,jlorenzo
https://hg.mozilla.org/integration/autoland/rev/2c41b39e150b
[taskgraph] Fix erroneous 'tasks_for: actions' in test parameter files, r=releng-reviewers,jlorenzo
https://hg.mozilla.org/integration/autoland/rev/23aef6f0dc33
[taskgraph] Use an 'enable_always_target' param to control when 'always_target' should apply, r=releng-reviewers,gbrown

Comment 18

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/80eac7743f74
https://hg.mozilla.org/mozilla-central/rev/3df4832d21a0
https://hg.mozilla.org/mozilla-central/rev/2c41b39e150b
https://hg.mozilla.org/mozilla-central/rev/23aef6f0dc33