feat(graph-sequencer): add package @pnpm/graph-sequencer #7168

ahaoboy · 2023-10-07T07:30:58Z

support Subgraph Sorting for Improved Performance

Faster than https://github.com/jamiebuilds/graph-sequencer

Subgraph Sort

In scenarios where we have a large graph but only need to execute commands on a few specific nodes, the graphSequencer function now offers enhanced support. The includeNodes parameter allows us to limit the result to only the nodes we're interested in.

graphSequencer(new Map([
    [0, [1]],
    [1, [2]],
    [2, [3]],
    [3, [0]],
]), [0, 1, 2])

For instance, if our graph is a big cycle but our subgraph is a simple linked list, we can now safely execute commands on the selected nodes.

{
    safe: true,
    chunks: [[2], [1], [0]],
    cycles: [],
}

Improved Behavior

In the previous behavior of the graphSequencer function, there were some peculiarities when handling single-node cycles.

graphSequencer({
  graph: new Map([
    ['a', ['a']],
    ['b', ['b']],
    ['c', ['c']],
  ]),
  groups: ['a', 'b', 'c']
})

This resulted in issues when dealing with single-node cycles, as demonstrated by the following output:

{
  safe: false,
  chunks: [ [ 'a' ], [ 'b' ], [ 'c' ] ],
  cycles: [
    [ 'a' ], [ 'a', 'a' ],
    [ 'b' ], [ 'b', 'b' ],
    [ 'c' ], [ 'c', 'c' ],
    [ 'b' ], [ 'c' ],
    [ 'c' ]
  ]
}

Now, with the improved behavior, each single-node cycle is treated independently, ensuring safe parallel execution:

{
  safe: true,
  chunks: [ [ 'a', 'b', 'c' ] ],
  cycles: [ [ 'a' ], [ 'b' ], [ 'c' ] ]
}

welcome · 2023-10-07T07:31:00Z

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

zkochan · 2023-10-07T11:53:54Z

Make the change here https://github.com/pnpm/graph-sequencer instead of moving the project to the monorepo. This way I can't understand what the changes were.

ahaoboy · 2023-10-09T02:49:52Z

This PR represents a reimplementation of the old repository, addressing previously unsupported scenarios and resolving issues related to unexpected behavior. It also includes performance enhancements. The changes have been successfully validated against the original tests and additional test cases. The reason for not submitting this PR to the old repository is due to its lack of support for TypeScript (ts), linting, and other modern tooling. Should I limit my modifications to the algorithmic aspects within the framework of the original repository?

Make the change here https://github.com/pnpm/graph-sequencer instead of moving the project to the monorepo. This way I can't understand what the changes were.

zkochan · 2023-10-09T09:54:02Z

The tests are failing

ahaoboy · 2023-10-10T03:56:51Z

@zkochan done

workspace/graph-sequencer/src/index.ts

zkochan · 2023-10-14T20:03:11Z

workspace/sort-packages/src/index.ts

-    graph,
-    groups: [keys],
-  })
+  return graphSequencer(graph, keys)


why has the API of the function changed?

In the older version, the 'groups' parameter more like a concept of priority. In the case of cycles within the graph, if the nodes within a cycle have different execution priorities, it is safe to execute that cycle in the order of priority. For example:

graphSequencer({ graph: new Map([ ['a', ['b']], ['b', ['c']], ['c', ['a']], ]), groups: [['a', 'b', 'c']] })

If we use the same priority, the cycle is unsafe:

{ safe: false, chunks: [ [ 'a' ], [ 'c' ], [ 'b' ] ], cycles: [ [ 'a', 'b', 'c' ] ] }

However, if we use different priorities:

groups: [['a'], ['b', 'c']]

The execution becomes safe:

{ safe: true, chunks: [ [ 'a' ], [ 'c' ], [ 'b' ] ], cycles: [] }

Currently, pnpm does not have the concept of priorities. All nodes passed as parameters have the same priority when called. Priority is not supported at the moment, but support for subgraphs (currently also requiring all nodes to be passed) is available. If you wish to add priorities in the future, it's a straightforward process - you just need to implement priority checking when identifying cycles.

zkochan · 2023-10-14T20:04:18Z

workspace/sort-packages/src/index.ts

-        But we really want 'a' to be executed first.
-        */
-        setOfKeys.has(d))]
+        d => d !== pkgPath && setOfKeys.has(d))]


why were the comments removed?

Because both scenarios are now supported

Still retained d => d !== pkgPath && setOfKeys.has(d))], this is because it reduces the number of unnecessary cycles and handles external nodes. If the 'includeNodes' parameter contains nodes that are not within the graph, we cannot determine whether to discard them or treat them as separate isolated nodes. This decision should be made by the caller. If they should be discarded, they should be removed from 'includeNodes,' and if they are to be considered as standalone nodes within the graph, they should be added to the graph.

workspace/graph-sequencer/test/index.ts

zkochan · 2023-10-14T20:13:21Z

workspace/graph-sequencer/test/index.ts

+test('graph with multiple dependencies on one item', () => {
+  expect(graphSequencer(new Map([
+    [0, [3]],
+    [1, [3]],
+    [2, []],
+    [3, []],
+  ]), [0, 1, 2, 3])).toStrictEqual(
+    {
+      safe: true,
+      chunks: [[2, 3], [0, 1]],
+      cycles: [],
+    }
+  )
+})
+


This is not related to your changes but I guess our script runners are not as affective as they should be at the moment.

We don't really need to wait for both 2 and 3 to finish before running 0 an 1. We only need to wait for 3 to finish and then we can continue with 0 and 1 while 2 might be still in progress.

The behavior is the same as the old version

graphSequencer({ graph: new Map([ ['a', ['c']], ['b', ['c']], ['c', []], ['d', []], ]), groups: [['a', 'b', 'c','d']] }) // { safe: true, chunks: [ [ 'c', 'd' ], [ 'a', 'b' ] ], cycles: [] }

workspace/graph-sequencer/test/index.ts

ahaoboy · 2023-10-15T04:03:59Z

@zkochan All done, PTAL

zkochan · 2023-10-15T09:39:04Z

CONTRIBUTING.md

@@ -36,7 +36,7 @@ sudo dnf install make automake gcc gcc-c++ kernel-devel
 You can run the tests of the project that you modified by going to the project's directory and running:

 ```shell
-pnpm test
+pnpm test-main


This should be reverted.

There is no npm command named 'test' in root project

It is not about the root project.

The text says "by going to the project's directory"

zkochan · 2023-10-15T10:12:05Z

Let's move the graph-sequencer project from the workspace to the deps directory. Also, rename it to @pnpm/deps.graph-sequencer.

zkochan · 2023-10-15T12:43:17Z