Ensure if-dependency edges don't pull in unselected tasks during target phase

[ahal]

No description provided.

[Eijebong]

Does this fix #710 ?

[ahal]

Yes! Forgot about that.. I'll add it to the commit message

[ahal]

Hm, though it would only impact the if-dependencies.. Should we use the same logic for soft-dependencies?

I think I'd want to do it in a follow-up as there is some time pressure to fixing this for if-dependencies. The bug is currently causing some havoc in Gecko.

[Eijebong]

I think this is slightly wrong? If a task is both an if-dep for one task and a hard dep for another, the if-dep edge gets dropped.

def test_if_deps_bork(maketgg):
    tgg = maketgg(
        target_tasks=["u-t-0", "v-t-0"],
        kinds=[
            ("base", {}),
            (
                "v",
                {
                    "task-defaults": {
                        "dependencies": {"base": "base-t-0"},
                    }
                },
            ),
            (
                "u",
                {
                    "task-defaults": {
                        "dependencies": {"if-dep": "base-t-0"},
                        "if-dependencies": ["base-t-0"],
                    }
                },
            ),
        ],
    )

    target_graph = tgg.target_task_graph

    assert "u-t-0" in target_graph.tasks
    assert "v-t-0" in target_graph.tasks
    assert "base-t-0" in target_graph.tasks

    edges = target_graph.graph.edges
    assert ("u-t-0", "base-t-0", "if-dep") in edges

[jcristau]

I worry a bit about unintended consequences here, for tasks that might expect that if they're running then all their dependencies have also run, not just one of them.

[ahal]

I think this is slightly wrong? If a task is both an if-dep for one task and a hard dep for another, the if-dep edge gets dropped.

Is that wrong though? Tbh, that sounds like intuitively what I'd expect. But I guess part of the problem is that this behaviour is all undefined and everyone has a slightly different idea of what's intuitive.

But specifically to this question, why should the existence of a hard dep have any bearing on the how the if-deps are treated?

[ahal]

I worry a bit about unintended consequences here, for tasks that might expect that if they're running then all their dependencies have also run, not just one of them.

For sure, definitely a backwards incompatible change. I guess we could pretty easily audit Gecko to see where if-dependencies are actually being used.

In practice I don't think this will be a huge issue though? Like if a task has that expectation it feels like they shouldn't be using if-dependencies in the first place because I believe it's already possible for this to not be the case depending on how the optimization shakes out.

[Eijebong]

But specifically to this question, why should the existence of a hard dep have any bearing on the how the if-deps are treated?

Because both u-t-0 and base-t-0 are in the graph, with base-t-0 being brought up not as target but because of v-t-0. I'd expect a task that defines something as a dependency to keep the edge on said dependency (which it doesn't).

[ahal]

I believe it's already possible for this to not be the case depending on how the optimization shakes out.

Actually this appears to be incorrect. I wrote a quick test to try it out and as long as one if-dep doesn't get optimized away, the others appear to get pulled in as well..

This feels surprising and wrong to me, but maybe I'm not thinking of the use cases for it properly?

[ahal]

Because both u-t-0 and base-t-0 are in the graph, with base-t-0 being brought up not as target but because of v-t-0. I'd expect a task that defines something as a dependency to keep the edge on said dependency (which it doesn't).

Oh I see, that makes sense. Before fixing this edge case however, I think we need to agree on what the behaviour of if-dependencies ought to be. As is this patch makes what happens in the target phase inconsistent with what happens at the optimization phase.. which I'm not in favour of.

Intuitively I kind of think we should fix the optimization phase to follow this PR. But is it worth breaking backwards compat? If we decide not to fix this, it does mean we need to stop using if-dependencies for code coverage in Gecko.