Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 of 4 tasks
JacobLey opened this issue Jul 14, 2024 · 8 comments · Fixed by #28542
Closed
1 of 4 tasks

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

JacobLey opened this issue Jul 14, 2024 · 8 comments · Fixed by #28542
Assignees
Labels

Comments

@JacobLey
Copy link
Contributor

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
  1. Run nx graph
  • Confirm that the report-impl task has both build-imp and test-impl as dependencies
  1. 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

  • macOS
  • Linux
  • Windows
  • Other (Please specify)

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

@FrozenPandaz FrozenPandaz added the scope: core core nx functionality label Jul 16, 2024
JacobLey added a commit to JacobLey/leyman that referenced this issue Jul 23, 2024
@jonathanmorley
Copy link
Contributor

This appears to have been introduced in 19.1.1

@jonathanmorley
Copy link
Contributor

I suspect its caused by this #26033

@xiongemi
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

i think the task graph is wrong

@JacobLey
Copy link
Contributor Author

JacobLey commented Oct 2, 2024

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
Copy link
Collaborator

xiongemi commented Oct 21, 2024

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:

export function isCompatibleTarget(

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}"
      }
    },

xiongemi added a commit that referenced this issue Oct 24, 2024
)

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

this pr is a follow up to #26033.
for function `createTaskGraph`, it should not accept
`defaultDependencyConfigs`, it should be `extraTargetDependencies`.

## Current Behavior
<!-- This is the behavior we have today -->
this caused an issue where the task graph shows that 2 tasks depend on
each other; however, when actually running, it does not.

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes #26929
jaysoo pushed a commit that referenced this issue Oct 25, 2024
)

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

this pr is a follow up to #26033.
for function `createTaskGraph`, it should not accept
`defaultDependencyConfigs`, it should be `extraTargetDependencies`.

## Current Behavior
<!-- This is the behavior we have today -->
this caused an issue where the task graph shows that 2 tasks depend on
each other; however, when actually running, it does not.

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes #26929
Copy link

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants