Skip to content

Commit e8beb31

Browse files
Ensure deterministic graph calculation with consistent layer, node, and edge ordering in Kedro-Viz (#2185)
Resolves #2057. Based on @astrojuanlu's comment, I realized that Kedro-Viz might be causing the randomness. To investigate, I checked if the inputs to graph calculations—nodes, edges, and layout—were consistent. While the order of nodes and layout remained stable, the order of edges varied with each backend data request. To address this, we now sort the layers, the edges and nodes to ensure consistency. Update - As @ravi-kumar-pilla noted, there was an issue with layer ordering: changing a node name could alter the layer order, especially for layers with identical dependencies, like model_input and feature in the example he shared. This issue stemmed from the layer_dependencies dictionary in services/layers.py, where layers with the same dependencies weren’t consistently ordered. To fix this, I added alphabetical sorting for layers with identical dependencies to ensure stability in toposort. For nodes and edges, I now sort them immediately upon loading from the backend API, ensuring they are consistently ordered in the Redux state. For testing, I initially considered using Cypress/backend e2e testing with screenshot comparison of the flowchart, but it proved too complex. Instead, I created a new mock dataset, reordered_spaceflights, with reordered nodes and edges. I added tests in normalise-data-test and actions/graph-test. The first test verifies that nodes and edges are consistently sorted by their ids, regardless of backend order. The second test compares the x and y coordinates in the flowchart, confirming that the graph layout is the same between the two mocks.
1 parent 5a7f11a commit e8beb31

File tree

8 files changed

+434
-19
lines changed

8 files changed

+434
-19
lines changed

package/kedro_viz/services/layers.py

+5
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,11 @@ def find_child_layers(node_id: str) -> Set[str]:
106106
if layer not in layer_dependencies:
107107
layer_dependencies[layer] = set()
108108

109+
# Sort `layer_dependencies` keys for consistent ordering of layers with the same dependencies
110+
layer_dependencies = defaultdict(
111+
set, {k: layer_dependencies[k] for k in sorted(layer_dependencies)}
112+
)
113+
109114
# Use graphlib.TopologicalSorter to sort the layer dependencies.
110115
try:
111116
sorter = TopologicalSorter(layer_dependencies)

package/tests/test_services/test_layers.py

+27-1
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,32 @@
154154
{"node_1": {}, "node_2": {}},
155155
["a", "b"],
156156
),
157+
(
158+
# Case where if two layers e.g. `int` and `primary` layers share the same dependencies, they get sorted alphabetically.
159+
"""
160+
node_1(layer=raw) -> node_3(layer=int)
161+
node_2(layer=raw) -> node_4(layer=primary)
162+
node_3(layer=int) -> node_5(layer=feature)
163+
node_4(layer=primary) -> node_6(layer=feature)
164+
""",
165+
{
166+
"node_1": {"id": "node_1", "layer": "raw"},
167+
"node_2": {"id": "node_2", "layer": "raw"},
168+
"node_3": {"id": "node_3", "layer": "int"},
169+
"node_4": {"id": "node_4", "layer": "primary"},
170+
"node_5": {"id": "node_5", "layer": "feature"},
171+
"node_6": {"id": "node_6", "layer": "feature"},
172+
},
173+
{
174+
"node_1": {"node_3"},
175+
"node_2": {"node_4"},
176+
"node_3": {"node_5"},
177+
"node_4": {"node_6"},
178+
"node_5": set(),
179+
"node_6": set(),
180+
},
181+
["raw", "int", "primary", "feature"],
182+
),
157183
],
158184
)
159185
def test_sort_layers(graph_schema, nodes, node_dependencies, expected):
@@ -170,7 +196,7 @@ def test_sort_layers(graph_schema, nodes, node_dependencies, expected):
170196
for node_id, node_dict in nodes.items()
171197
}
172198
sorted_layers = sort_layers(nodes, node_dependencies)
173-
assert sorted(sorted_layers) == sorted(expected), graph_schema
199+
assert sorted_layers == expected, graph_schema
174200

175201

176202
def test_sort_layers_should_return_empty_list_on_cyclic_layers(mocker):

src/actions/graph.test.js

+61-17
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,37 @@
11
import { createStore } from 'redux';
22
import reducer from '../reducers';
3-
import { mockState } from '../utils/state.mock';
43
import { calculateGraph, updateGraph } from './graph';
54
import { getGraphInput } from '../selectors/layout';
5+
import { prepareState } from '../utils/state.mock';
6+
import spaceflights from '../utils/data/spaceflights.mock.json';
7+
import spaceflightsReordered from '../utils/data/spaceflights_reordered.mock.json';
8+
import { toggleModularPipelinesExpanded } from '../actions/modular-pipelines';
69

710
describe('graph actions', () => {
11+
const getMockState = (data) =>
12+
prepareState({
13+
data,
14+
beforeLayoutActions: [
15+
() =>
16+
toggleModularPipelinesExpanded(['data_science', 'data_processing']),
17+
],
18+
});
19+
820
describe('calculateGraph', () => {
921
it('returns updateGraph action if input is falsey', () => {
1022
expect(calculateGraph(null)).toEqual(updateGraph(null));
1123
});
1224

1325
it('sets loading to true immediately', () => {
14-
const store = createStore(reducer, mockState.spaceflights);
26+
const store = createStore(reducer, getMockState(spaceflights));
1527
expect(store.getState().loading.graph).not.toBe(true);
16-
calculateGraph(getGraphInput(mockState.spaceflights))(store.dispatch);
28+
calculateGraph(getGraphInput(getMockState(spaceflights)))(store.dispatch);
1729
expect(store.getState().loading.graph).toBe(true);
1830
});
1931

2032
it('sets loading to false and graph visibility to true after finishing calculation', () => {
21-
const store = createStore(reducer, mockState.spaceflights);
22-
return calculateGraph(getGraphInput(mockState.spaceflights))(
33+
const store = createStore(reducer, getMockState(spaceflights));
34+
return calculateGraph(getGraphInput(getMockState(spaceflights)))(
2335
store.dispatch
2436
).then(() => {
2537
const state = store.getState();
@@ -29,19 +41,51 @@ describe('graph actions', () => {
2941
});
3042

3143
it('calculates a graph', () => {
32-
const state = Object.assign({}, mockState.spaceflights);
33-
delete state.graph;
34-
const store = createStore(reducer, state);
44+
const initialState = { ...getMockState(spaceflights), graph: {} };
45+
const store = createStore(reducer, initialState);
3546
expect(store.getState().graph).toEqual({});
36-
return calculateGraph(getGraphInput(state))(store.dispatch).then(() => {
37-
expect(store.getState().graph).toEqual(
38-
expect.objectContaining({
39-
nodes: expect.any(Array),
40-
edges: expect.any(Array),
41-
size: expect.any(Object),
42-
})
43-
);
44-
});
47+
return calculateGraph(getGraphInput(initialState))(store.dispatch).then(
48+
() => {
49+
expect(store.getState().graph).toEqual(
50+
expect.objectContaining({
51+
nodes: expect.any(Array),
52+
edges: expect.any(Array),
53+
size: expect.any(Object),
54+
})
55+
);
56+
}
57+
);
58+
});
59+
60+
it('compares deterministic flowchart of two differently ordered same projects', () => {
61+
const store1 = createStore(reducer, getMockState(spaceflights));
62+
const store2 = createStore(reducer, getMockState(spaceflightsReordered));
63+
64+
return calculateGraph(getGraphInput(getMockState(spaceflights)))(
65+
store1.dispatch
66+
)
67+
.then(() =>
68+
calculateGraph(getGraphInput(getMockState(spaceflightsReordered)))(
69+
store2.dispatch
70+
)
71+
)
72+
.then(() => {
73+
// Get node coordinates for both graphs
74+
const graph1Coords = store1.getState().graph.nodes.map((node) => ({
75+
id: node.id,
76+
x: node.x,
77+
y: node.y,
78+
}));
79+
80+
const graph2Coords = store2.getState().graph.nodes.map((node) => ({
81+
id: node.id,
82+
x: node.x,
83+
y: node.y,
84+
}));
85+
86+
// Verify coordinates consistency between both graphs
87+
expect(graph1Coords).toEqual(expect.arrayContaining(graph2Coords));
88+
});
4589
});
4690
});
4791
});

src/selectors/sliced-pipeline.test.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,13 @@ describe('Selectors', () => {
1212
const expected = [
1313
'23c94afb',
1414
'47b81aa6',
15+
'90ebe5f3',
1516
'daf35ba0',
1617
'c09084f2',
1718
'0abef172',
1819
'e5a9ec27',
1920
'b7bb7198',
2021
'f192326a',
21-
'90ebe5f3',
2222
'data_processing',
2323
];
2424
const newState = reducer(mockState.spaceflights, {

src/store/normalize-data.js

+10
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,15 @@ const getNodeTypesFromUrl = (state, typeQueryParams) => {
250250
return state;
251251
};
252252

253+
/**
254+
* Sort the edges, nodes in the state object to ensure deterministic graph layout
255+
* @param {Object} state The state object to sort
256+
*/
257+
const sortNodesEdges = (state) => {
258+
state.edge?.ids?.sort((a, b) => a.localeCompare(b));
259+
state.node?.ids?.sort((a, b) => a.localeCompare(b));
260+
};
261+
253262
/**
254263
* Updates the state with filters from the URL.
255264
* @param {Object} state - State object
@@ -331,6 +340,7 @@ const normalizeData = (data, expandAllPipelines) => {
331340
data.layers.forEach(addLayer(state));
332341
}
333342

343+
sortNodesEdges(state);
334344
const updatedState = updateStateWithFilters(state, data.tags);
335345
return updatedState;
336346
};

src/store/normalize-data.test.js

+17
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import normalizeData, { createInitialPipelineState } from './normalize-data';
22
import spaceflights from '../utils/data/spaceflights.mock.json';
3+
import spaceflightsReordered from '../utils/data/spaceflights_reordered.mock.json';
34

45
const initialState = createInitialPipelineState();
56

@@ -90,4 +91,20 @@ describe('normalizeData', () => {
9091
expect(node).toHaveProperty('name');
9192
});
9293
});
94+
95+
it('should have identical nodes and edges, in the same order, regardless of the different ordering from the api', () => {
96+
// Normalize both datasets
97+
const initialState = normalizeData(spaceflights, true);
98+
const reorderedState = normalizeData(spaceflightsReordered, true);
99+
100+
// Compare nodes and edges by converting to JSON for deep equality
101+
// Directly compare specific properties of nodes and edges, ensuring order and content
102+
expect(initialState.node.ids).toEqual(reorderedState.node.ids);
103+
expect(initialState.node.name).toEqual(reorderedState.node.name);
104+
expect(initialState.node.type).toEqual(reorderedState.node.type);
105+
106+
expect(initialState.edge.ids).toEqual(reorderedState.edge.ids);
107+
expect(initialState.edge.sources).toEqual(reorderedState.edge.sources);
108+
expect(initialState.edge.targets).toEqual(reorderedState.edge.targets);
109+
});
93110
});

0 commit comments

Comments
 (0)