nrwl / nx

Smart Monorepos · Fast CI
https://nx.dev
MIT License
23.62k stars 2.36k forks source link

Nx executes targets out of order when overriding target in `project.json` #26929

Closed JacobLey closed 2 weeks ago

JacobLey commented 3 months ago

Current Behavior

When defining a task graph that has tasks defined in both nx.json and project.json, it is expected that the dependsOn is inherited by the nx.json implemention if not otherwise overridden by project.json.

In fact, this is consistent with the task graph displayed by nx graph.

However, when overriding targets in project.json, the dependency order is not always respected. This conflicts with the graph which claims it still is.

Create two "meta-targets" build and test. These are simply nx:noop executors that point to a series of real targets for user convenicence.

Building is bound to a single target build-impl. Testing is bound to two targets test-impl and report-impl. These both depend on build being complete.

For simplicity, these default implementations are a 1 second wait, then logging their name.

report-impl is overwritten in project.json to just immediately log the name.

Inspect the task graph nx graph appears to confirm that a test command would:

  1. Run build
  2. Run test
  3. Generate report

Running nx run foo:test actually results in the following logs:

  1. REPORT

  2. BUILD

  3. TEST

The report step is executed immediately, despite the dependency graph claiming otherwise.

Expected Behavior

Running nx run foo:test should result in the following logs:

  1. BUILD

  2. TEST

  3. REPORT

GitHub Repo

https://github.com/JacobLey/issue-recreator/tree/nx-task-dependency-order

Steps to Reproduce

  1. Set up package as described above, or in example repo's README
  2. pnpm i
    • Install necessary packages
  3. Run nx graph
    • Confirm that the report-impl task has both build-imp and test-impl as dependencies
  4. nx run foo:test
    • Inspect the console output shows REPORT before BUILD (and TEST), meaning that it did not properly block on a dependency.

Nx Report

Node   : 22.4.1
OS     : linux-arm64
pnpm   : 9.5.0

nx (global)  : 19.4.3
nx           : 19.4.3
@nrwl/tao    : 19.4.3

Failure Logs

No failure logs, just unexpected execution order.

This _can_ easily induce unrelated failure logs, like in the example above trying to execute tests before the build even starts.

Package Manager Version

pnpm 9.5.0

Operating System

Additional Information

I've tried reducing the example case and don't always see it, but definitely experiencing it 10-fold in my live repo.

Attached is image of nx graph. While the dependency explanation above may seem a bit unclear, hopefully it is fairly obvious here that a fairly linear execution of tasks is expected here. I agree with the output provided by nx graph, it is the actual execution of run that is out of order.

Screenshot 2024-07-13 at 9 54 49 PM
jonathanmorley commented 2 months ago

This appears to have been introduced in 19.1.1

jonathanmorley commented 2 months ago

I suspect its caused by this https://github.com/nrwl/nx/pull/26033

xiongemi commented 1 month ago

it is actually expected behaviour that dependsOn in project.json overriding the one in nx.json. For the repo, in project.json, for report-impl, since there is no dependsOn specified, it will take that value. So it would consider report-impl does not have any dependencies, hence it runs first. You can copy the dependsOn value from nx.json to project.json and it will run after test-impl.

JacobLey commented 1 month ago

dependsOn in project.json overriding the one in nx.json

Sure, if I specify dependsOn I expect it to override. That is generally consistent with the rest of Nx behavior.

since there is no dependsOn specified, it will take that value

Do you have any documentation to support that? I have not seen anything on those lines, and in fact explicitly see the opposite behavior when testing. Omission of a field is exactly what should trigger inheritance, otherwise how is it ever possible to inherit the default?

I extended my example repo with trivial (nx:noop-backed) tasks a + b. b depends on a, but that is only declared in the nx.json, and project.json re-declares the tasks but omits the dependsOn.

nx.json

{
    "$schema": "./node_modules/nx/schemas/nx-schema.json",
    "cli": { "packageManager": "pnpm" },
    "targetDefaults": {
        "a": {
            "executor": "nx:noop"
        },
        "b": {
            "executor": "nx:noop",
            "dependsOn": ["a"]
        }
    }
}

project.json

{
  "$schema": "../../node_modules/nx/schemas/project-schema.json",
  "name": "foo",
  "targets": {

    "a": {
      "executor": "nx:noop"
    },
    "b": {
      "executor": "nx:noop"
    }
  }
}

Result of nx graph (dependency is maintained):

Screenshot 2024-09-17 at 11 11 09 AM
xiongemi commented 1 month ago

i think the task graph is wrong

JacobLey commented 1 month ago

If literally everything (including nothing) overrides the default dependencies, what is the point of having them?

I still believe the expected behavior is to inherit dependsOn (when omitted), but if it truly is expected to be overridden every time, then I would change my request on this issue to:

  1. Remove dependsOn from nx.json's targetDefaults
  2. Fix task graph to no longer inherit the value
xiongemi commented 2 weeks ago

I submitted a PR to fix the graph issue: https://github.com/nrwl/nx/pull/28542/files. After merging this PR, the task graph should reflect the order in which tasks are run.

The issue with the target in project.json overrides the one in nx.json, not merging these two because the command is different from these two. from logic: https://github.com/nrwl/nx/blob/e57b85152cc51685ddfe71e376dce6802407c0f5/packages/nx/src/project-graph/utils/project-configuration-utils.ts#L908 These two are not compatible targets. For compatible targets, both the executor and command have to be the same. Your project.json target looks like:

    "report-impl": {
      "options": {
        "commands": ["echo REPORT"],
        "color": true,
        "cwd": "{projectRoot}"
      }
    },

and nx.json is like:

        "report-impl": {
            "executor": "nx:run-commands",
            "options": {
              "commands": ["sleep 1", "echo REPORT"],
              "color": true,
              "cwd": "{projectRoot}"
            },
            "dependsOn": ["test:_", "test-impl"]
        },

They have same executor but different command.

There are use cases where the commands are completely different (not a simple echo), so merging the options does not make sense. The quick hack would just be to remove the executor defined in the project.json's target to something like, so your project.json target options will be merged with default one in nx.json:

    "report-impl": {
      "options": {
        "commands": ["echo REPORT"],
        "color": true,
        "cwd": "{projectRoot}"
      }
    },