From 9a7e812947eb40bde272c81b7e41eb63cf1bbe47 Mon Sep 17 00:00:00 2001 From: Emily Xiong Date: Wed, 6 Nov 2024 11:49:59 -0800 Subject: [PATCH] fix(core): task graph needs to handle multiple cycles (#28793) ## Current Behavior - getNonDummyDeps is a recursive function. we pass in cycle arg to this function. currently we end the recursion when the task is in a cycle. however, currently there is an error "Maximum call stack size". i suspect this recursion is not being ended because the cycle is not detected when there are multiple cycles. - add a function to get all cycles of the graph - also, change getNonDummyDeps to track a list of seen tasks, even with no cycle detected, this function will not run into infinite recursion ## Expected Behavior ## Related Issue(s) Fixes https://github.com/nrwl/nx/issues/28788 (cherry picked from commit 9c245667c242aaeef808e18514bfcf1b2bdcd7fe) --- .../tasks-runner/create-task-graph.spec.ts | 329 +++++++++++++++++- .../nx/src/tasks-runner/create-task-graph.ts | 114 +++--- .../src/tasks-runner/task-graph-utils.spec.ts | 82 ++++- .../nx/src/tasks-runner/task-graph-utils.ts | 29 +- 4 files changed, 487 insertions(+), 67 deletions(-) diff --git a/packages/nx/src/tasks-runner/create-task-graph.spec.ts b/packages/nx/src/tasks-runner/create-task-graph.spec.ts index a327f484919a5..0f45f49f1cd48 100644 --- a/packages/nx/src/tasks-runner/create-task-graph.spec.ts +++ b/packages/nx/src/tasks-runner/create-task-graph.spec.ts @@ -4,7 +4,11 @@ import { ProjectGraphProjectNode, } from '../config/project-graph'; import { ProjectConfiguration } from '../config/workspace-json-project-json'; -import { createTaskGraph } from './create-task-graph'; +import { + createTaskGraph, + filterDummyTasks, + getNonDummyDeps, +} from './create-task-graph'; describe('createTaskGraph', () => { let projectGraph: ProjectGraph; @@ -2657,6 +2661,150 @@ describe('createTaskGraph', () => { }, }); }); + + it('should handle dependencies with 2 cycles (app1->app2<->app3->app4, app5->app6<->app7->app8)', () => { + projectGraph = { + nodes: { + app1: { + name: 'app1', + type: 'app', + data: { + root: 'app1-root', + targets: { + compile: { + executor: 'nx:run-commands', + dependsOn: ['precompiple', '^precompile'], + }, + }, + }, + }, + app2: { + name: 'app2', + type: 'app', + data: { + root: 'app2-root', + targets: { + compile: { + executor: 'nx:run-commands', + dependsOn: ['precompiple', '^precompile'], + }, + }, + }, + }, + app3: { + name: 'app3', + type: 'app', + data: { + root: 'app3-root', + targets: { + compile: { + executor: 'nx:run-commands', + dependsOn: ['precompiple', '^precompile'], + }, + }, + }, + }, + app4: { + name: 'app4', + type: 'app', + data: { + root: 'app4-root', + targets: { + precompile: { + executor: 'nx:run-commands', + }, + }, + }, + }, + app5: { + name: 'app5', + type: 'app', + data: { + root: 'app5-root', + targets: { + compile: { + executor: 'nx:run-commands', + dependsOn: ['precompiple', '^precompile'], + }, + }, + }, + }, + app6: { + name: 'app6', + type: 'app', + data: { + root: 'app6-root', + targets: { + compile: { + executor: 'nx:run-commands', + dependsOn: ['precompiple', '^precompile'], + }, + }, + }, + }, + app7: { + name: 'app7', + type: 'app', + data: { + root: 'app7-root', + targets: { + compile: { + executor: 'nx:run-commands', + dependsOn: ['precompiple', '^precompile'], + }, + }, + }, + }, + app8: { + name: 'app8', + type: 'app', + data: { + root: 'app8-root', + targets: { + precompile: { + executor: 'nx:run-commands', + }, + }, + }, + }, + }, + dependencies: { + app1: [{ source: 'app1', target: 'app2', type: 'implicit' }], + app2: [{ source: 'app2', target: 'app3', type: 'implicit' }], + app3: [ + { source: 'app3', target: 'app4', type: 'implicit' }, + { source: 'app3', target: 'app2', type: 'implicit' }, + ], + app5: [{ source: 'app5', target: 'app6', type: 'implicit' }], + app6: [{ source: 'app6', target: 'app7', type: 'implicit' }], + app7: [ + { source: 'app7', target: 'app8', type: 'implicit' }, + { source: 'app7', target: 'app6', type: 'implicit' }, + ], + }, + }; + + let taskGraph = createTaskGraph( + projectGraph, + {}, + ['app1', 'app2', 'app3', 'app5', 'app6', 'app7'], + ['compile'], + 'development', + { + __overrides_unparsed__: [], + } + ); + expect(taskGraph.dependencies).toEqual({ + 'app1:compile': [], + 'app2:compile': [], + 'app3:compile': ['app4:precompile'], + 'app4:precompile': [], + 'app5:compile': [], + 'app6:compile': [], + 'app7:compile': ['app8:precompile'], + 'app8:precompile': [], + }); + }); }); class GraphBuilder { @@ -2699,3 +2847,182 @@ class GraphBuilder { }; } } + +describe('filterDummyTasks', () => { + it('should filter out dummy tasks', () => { + const dependencies = { + 'app1:compile': ['app2:__nx_dummy_task__'], + 'app2:__nx_dummy_task__': ['app3:__nx_dummy_task__'], + 'app3:__nx_dummy_task__': ['app4:__nx_dummy_task__'], + 'app4:__nx_dummy_task__': ['app5:build'], + 'app5:build': [], + }; + filterDummyTasks(dependencies); + expect(dependencies).toEqual({ + 'app1:compile': ['app5:build'], + 'app5:build': [], + }); + }); + + it('should filter out dummy tasks with 1 cycle', () => { + const dependencies = { + 'app1:compile': ['app2:__nx_dummy_task__'], + 'app2:__nx_dummy_task__': ['app3:__nx_dummy_task__'], + 'app3:__nx_dummy_task__': [ + 'app4:__nx_dummy_task__', + 'app2:__nx_dummy_task__', + ], + 'app4:__nx_dummy_task__': ['app5:build'], + 'app5:build': [], + }; + filterDummyTasks(dependencies); + expect(dependencies).toEqual({ + 'app1:compile': [], + 'app5:build': [], + }); + }); + + it('should filter out dummy tasks with 2 cycles', () => { + const dependencies = { + 'app1:compile': ['app2:__nx_dummy_task__'], + 'app2:__nx_dummy_task__': ['app3:__nx_dummy_task__'], + 'app3:__nx_dummy_task__': [ + 'app4:__nx_dummy_task__', + 'app2:__nx_dummy_task__', + ], + 'app4:__nx_dummy_task__': ['app5:build'], + 'app5:build': [], + 'app5:compile': ['app6:__nx_dummy_task__'], + 'app6:__nx_dummy_task__': ['app7:__nx_dummy_task__'], + 'app7:__nx_dummy_task__': ['app8:precompile', 'app6:__nx_dummy_task__'], + 'app8:precompile': [], + }; + filterDummyTasks(dependencies); + expect(dependencies).toEqual({ + 'app1:compile': [], + 'app5:build': [], + 'app5:compile': [], + 'app8:precompile': [], + }); + }); + + it('should filter out dummy tasks with a large list of dependencies without cycles', () => { + const dependencies = { + 'app1:compile': ['app2:__nx_dummy_task__'], + 'app2:__nx_dummy_task__': ['app3:__nx_dummy_task__'], + 'app3:__nx_dummy_task__': ['app4:precompile'], + 'app4:precompile': ['app5:build'], + 'app5:build': ['app6:__nx_dummy_task__'], + 'app6:__nx_dummy_task__': ['app7:__nx_dummy_task__'], + 'app7:__nx_dummy_task__': ['app8:precompile'], + 'app8:precompile': ['app9:__nx_dummy_task__', 'app10:build'], + 'app9:__nx_dummy_task__': ['app10:__nx_dummy_task__'], + 'app10:__nx_dummy_task__': ['app11:__nx_dummy_task__'], + 'app10:build': ['app11:__nx_dummy_task__'], + 'app11:__nx_dummy_task__': ['app12:__nx_dummy_task__'], + 'app12:__nx_dummy_task__': ['app13:__nx_dummy_task__'], + 'app13:__nx_dummy_task__': ['app14:__nx_dummy_task__'], + 'app14:__nx_dummy_task__': ['app15:__nx_dummy_task__'], + 'app15:__nx_dummy_task__': ['app16:__nx_dummy_task__'], + 'app16:__nx_dummy_task__': ['app17:__nx_dummy_task__'], + 'app17:__nx_dummy_task__': ['app18:__nx_dummy_task__'], + 'app18:__nx_dummy_task__': ['app19:__nx_dummy_task__'], + 'app19:__nx_dummy_task__': ['app20:__nx_dummy_task__'], + 'app20:__nx_dummy_task__': ['app21:build'], + 'app21:build': [], + }; + filterDummyTasks(dependencies); + expect(dependencies).toEqual({ + 'app1:compile': ['app4:precompile'], + 'app4:precompile': ['app5:build'], + 'app5:build': ['app8:precompile'], + 'app8:precompile': ['app21:build', 'app10:build'], + 'app10:build': ['app21:build'], + 'app21:build': [], + }); + }); +}); + +describe('getNonDummyDeps', () => { + it('should return the non dummy dependencies', () => { + const dependencies = { + 'app1:compile': ['app2:__nx_dummy_task__'], + 'app2:__nx_dummy_task__': ['app3:__nx_dummy_task__'], + 'app3:__nx_dummy_task__': ['app4:__nx_dummy_task__'], + 'app4:__nx_dummy_task__': ['app5:build'], + 'app5:build': [], + }; + expect( + getNonDummyDeps( + 'app2:__nx_dummy_task__', + dependencies, + null, + new Set(['app1:compile']) + ) + ).toEqual(['app5:build']); + }); + + it('should return the non dummy dependencies with a cycle even no cycle arg got passed in', () => { + const dependencies = { + 'app1:compile': ['app2:__nx_dummy_task__'], + 'app2:__nx_dummy_task__': [ + 'app3:__nx_dummy_task__', + 'app8:precompile', + 'app5:build', + ], + 'app3:__nx_dummy_task__': ['app2:__nx_dummy_task__', 'app4:precompile'], + 'app4:precompile': ['app5:build'], + 'app5:build': ['app6:__nx_dummy_task__'], + 'app6:__nx_dummy_task__': ['app7:__nx_dummy_task__', 'app1:compile'], + 'app7:__nx_dummy_task__': ['app8:precompile'], + 'app8:precompile': [], + }; + expect(getNonDummyDeps('app2:__nx_dummy_task__', dependencies)).toEqual([ + 'app4:precompile', + 'app8:precompile', + 'app5:build', + ]); + expect(getNonDummyDeps('app3:__nx_dummy_task__', dependencies)).toEqual([ + 'app8:precompile', + 'app5:build', + 'app4:precompile', + ]); + expect(getNonDummyDeps('app6:__nx_dummy_task__', dependencies)).toEqual([ + 'app8:precompile', + 'app1:compile', + ]); + }); + + it('should handle a long list of dependencies without cycle', () => { + const dependencies = { + 'app1:compile': ['app2:__nx_dummy_task__'], + 'app2:__nx_dummy_task__': ['app3:__nx_dummy_task__'], + 'app3:__nx_dummy_task__': ['app4:precompile'], + 'app4:precompile': ['app5:build'], + 'app5:build': ['app6:__nx_dummy_task__'], + 'app6:__nx_dummy_task__': ['app7:__nx_dummy_task__'], + 'app7:__nx_dummy_task__': ['app8:precompile'], + 'app8:precompile': ['app9:__nx_dummy_task__', 'app10:build'], + 'app9:__nx_dummy_task__': ['app10:__nx_dummy_task__'], + 'app10:__nx_dummy_task__': ['app11:__nx_dummy_task__'], + 'app10:build': ['app11:__nx_dummy_task__'], + 'app11:__nx_dummy_task__': ['app12:__nx_dummy_task__'], + 'app12:__nx_dummy_task__': ['app13:__nx_dummy_task__'], + 'app13:__nx_dummy_task__': ['app14:__nx_dummy_task__'], + 'app14:__nx_dummy_task__': ['app15:__nx_dummy_task__'], + 'app15:__nx_dummy_task__': ['app16:__nx_dummy_task__'], + 'app16:__nx_dummy_task__': ['app17:__nx_dummy_task__'], + 'app17:__nx_dummy_task__': ['app18:__nx_dummy_task__'], + 'app18:__nx_dummy_task__': ['app19:__nx_dummy_task__'], + 'app19:__nx_dummy_task__': ['app20:__nx_dummy_task__'], + 'app20:__nx_dummy_task__': ['app21:build'], + 'app21:build': [], + }; + expect(getNonDummyDeps('app2:__nx_dummy_task__', dependencies)).toEqual([ + 'app4:precompile', + ]); + expect(getNonDummyDeps('app9:__nx_dummy_task__', dependencies)).toEqual([ + 'app21:build', + ]); + }); +}); diff --git a/packages/nx/src/tasks-runner/create-task-graph.ts b/packages/nx/src/tasks-runner/create-task-graph.ts index 45347a086722f..27e5c72a2b183 100644 --- a/packages/nx/src/tasks-runner/create-task-graph.ts +++ b/packages/nx/src/tasks-runner/create-task-graph.ts @@ -8,7 +8,7 @@ import { Task, TaskGraph } from '../config/task-graph'; import { TargetDefaults, TargetDependencies } from '../config/nx-json'; import { output } from '../utils/output'; import { TargetDependencyConfig } from '../config/workspace-json-project-json'; -import { findCycle } from './task-graph-utils'; +import { findCycles } from './task-graph-utils'; const DUMMY_TASK_TARGET = '__nx_dummy_task__'; @@ -84,7 +84,7 @@ export class ProcessTasks { } } - this.filterDummyTasks(); + filterDummyTasks(this.dependencies); for (const taskId of Object.keys(this.dependencies)) { if (this.dependencies[taskId].length > 0) { @@ -377,62 +377,6 @@ export class ProcessTasks { } return id; } - - /** - * this function is used to get the non dummy dependencies of a task recursively - * For example, when we have the following dependencies: - * { - * 'app1:compile': [ 'app2:__nx_dummy_task__' ], - * 'app2:__nx_dummy_task__': [ 'app3:__nx_dummy_task__' ], - * 'app3:__nx_dummy_task__': [ 'app4:precompile' ], - * 'app4:precompile': [] - * } - * getNonDummyDeps('app1:compile') will return ['app1:compile'] - * getNonDummyDeps('app2:__nx_dummy_task__') will return ['app4:precompile'] - * getNonDummyDeps('app3:__nx_dummy_task__') will return ['app4:precompile'] - * getNonDummyDeps('app4:precompile') will return ['app4:precompile'] - */ - private getNonDummyDeps( - currentTask: string, - originalTask: string, - cycle?: string[] - ): string[] { - if (currentTask === originalTask) { - return []; - } else if (currentTask.endsWith(DUMMY_TASK_TARGET)) { - if (cycle?.length && cycle?.includes(currentTask)) { - return []; - } - // if not a cycle, recursively get the non dummy dependencies - return ( - this.dependencies[currentTask]?.flatMap((dep) => - this.getNonDummyDeps(dep, originalTask, cycle) - ) ?? [] - ); - } else { - return [currentTask]; - } - } - - private filterDummyTasks() { - const cycle = findCycle({ dependencies: this.dependencies }); - for (const [key, deps] of Object.entries(this.dependencies)) { - if (!key.endsWith(DUMMY_TASK_TARGET)) { - const normalizedDeps = []; - for (const dep of deps) { - normalizedDeps.push(...this.getNonDummyDeps(dep, key, cycle)); - } - - this.dependencies[key] = normalizedDeps; - } - } - - for (const key of Object.keys(this.dependencies)) { - if (key.endsWith(DUMMY_TASK_TARGET)) { - delete this.dependencies[key]; - } - } - } } export function createTaskGraph( @@ -490,3 +434,57 @@ function interpolateOverrides( }); return interpolatedArgs; } + +/** + * This function is used to filter out the dummy tasks from the dependencies + * It will manipulate the dependencies object in place + */ +export function filterDummyTasks(dependencies: { [k: string]: string[] }) { + const cycles = findCycles({ dependencies }); + for (const [key, deps] of Object.entries(dependencies)) { + if (!key.endsWith(DUMMY_TASK_TARGET)) { + const normalizedDeps = []; + for (const dep of deps) { + normalizedDeps.push( + ...getNonDummyDeps(dep, dependencies, cycles, new Set([key])) + ); + } + + dependencies[key] = normalizedDeps; + } + } + + for (const key of Object.keys(dependencies)) { + if (key.endsWith(DUMMY_TASK_TARGET)) { + delete dependencies[key]; + } + } +} + +/** + * this function is used to get the non dummy dependencies of a task recursively + */ +export function getNonDummyDeps( + currentTask: string, + dependencies: { [k: string]: string[] }, + cycles?: Set, + seen: Set = new Set() +): string[] { + if (seen.has(currentTask)) { + return []; + } + seen.add(currentTask); + if (currentTask.endsWith(DUMMY_TASK_TARGET)) { + if (cycles?.has(currentTask)) { + return []; + } + // if not a cycle, recursively get the non dummy dependencies + return ( + dependencies[currentTask]?.flatMap((dep) => + getNonDummyDeps(dep, dependencies, cycles, seen) + ) ?? [] + ); + } else { + return [currentTask]; + } +} diff --git a/packages/nx/src/tasks-runner/task-graph-utils.spec.ts b/packages/nx/src/tasks-runner/task-graph-utils.spec.ts index 60e7190cd8b6d..8df7201012e7f 100644 --- a/packages/nx/src/tasks-runner/task-graph-utils.spec.ts +++ b/packages/nx/src/tasks-runner/task-graph-utils.spec.ts @@ -2,20 +2,15 @@ import '../internal-testing-utils/mock-fs'; import { vol } from 'memfs'; -import { join } from 'path'; import { findCycle, + findCycles, makeAcyclic, validateNoAtomizedTasks, } from './task-graph-utils'; -import { tmpdir } from 'os'; -import { workspaceRoot } from '../utils/workspace-root'; -import { cacheDir } from '../utils/cache-directory'; -import { Task } from '../config/task-graph'; -import { ProjectGraph } from '../config/project-graph'; describe('task graph utils', () => { - describe('findCycles', () => { + describe('findCycle', () => { it('should return a cycle if it is there', () => { expect( findCycle({ @@ -29,6 +24,20 @@ describe('task graph utils', () => { }, } as any) ).toEqual(['a', 'c', 'e', 'a']); + + expect( + findCycle({ + dependencies: { + a: ['b', 'c'], + b: ['d'], + c: ['a'], + d: [], + e: ['f'], + f: ['q'], + q: ['e'], + }, + } as any) + ).toEqual(['a', 'c', 'a']); }); it('should return null when no cycle', () => { @@ -47,6 +56,65 @@ describe('task graph utils', () => { }); }); + describe('findCycles', () => { + it('should return all cycles', () => { + expect( + findCycles({ + dependencies: { + a: ['b', 'c'], + b: ['d'], + c: ['e'], + d: [], + e: ['q', 'a'], + q: [], + }, + } as any) + ).toEqual(new Set(['a', 'c', 'e'])); + + expect( + findCycles({ + dependencies: { + a: ['b', 'c'], + b: ['d'], + c: ['a'], + d: [], + e: ['f'], + f: ['q'], + q: ['e'], + }, + } as any) + ).toEqual(new Set(['a', 'c', 'e', 'f', 'q'])); + expect( + findCycles({ + dependencies: { + a: ['b', 'c'], + b: ['d'], + c: ['f'], + d: ['a'], + e: [], + f: ['q'], + q: ['c'], + }, + } as any) + ).toEqual(new Set(['a', 'b', 'd', 'c', 'f', 'q'])); + }); + + it('should return null when no cycle', () => { + expect( + findCycles({ + dependencies: { + a: ['b', 'c'], + b: ['d'], + c: ['e'], + d: [], + e: ['q'], + q: [], + }, + } as any) + ).toEqual(null); + }); + }); + describe('makeAcyclic', () => { it('should remove cycles when they are there', () => { const graph = { diff --git a/packages/nx/src/tasks-runner/task-graph-utils.ts b/packages/nx/src/tasks-runner/task-graph-utils.ts index fe9a0e72984cf..020572e84af7d 100644 --- a/packages/nx/src/tasks-runner/task-graph-utils.ts +++ b/packages/nx/src/tasks-runner/task-graph-utils.ts @@ -7,7 +7,7 @@ function _findCycle( id: string, visited: { [taskId: string]: boolean }, path: string[] -) { +): string[] | null { if (visited[id]) return null; visited[id] = true; @@ -19,6 +19,10 @@ function _findCycle( return null; } +/** + * This function finds a cycle in the graph. + * @returns the first cycle found, or null if no cycle is found. + */ export function findCycle(graph: { dependencies: Record; }): string[] | null { @@ -35,6 +39,29 @@ export function findCycle(graph: { return null; } +/** + * This function finds all cycles in the graph. + * @returns a list of unique task ids in all cycles found, or null if no cycle is found. + */ +export function findCycles(graph: { + dependencies: Record; +}): Set | null { + const visited = {}; + const cycles = new Set(); + for (const t of Object.keys(graph.dependencies)) { + visited[t] = false; + } + + for (const t of Object.keys(graph.dependencies)) { + const cycle = _findCycle(graph, t, visited, [t]); + if (cycle) { + cycle.forEach((t) => cycles.add(t)); + } + } + + return cycles.size ? cycles : null; +} + function _makeAcyclic( graph: { dependencies: Record }, id: string,