-
Notifications
You must be signed in to change notification settings - Fork 112
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
Ensure deterministic graph calculation with consistent layer, node, and edge ordering in Kedro-Viz #2185
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
@rashidakanchwala I tested with both your branch and main, I made the changes roughly 5 to 7 times and took screenshot every time and compared, it look exactly same on your branch and on main its different every time. Its working for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rashidakanchwala Really cool, Not easy to fix such bugs. 🎉
Hi @rashidakanchwala , The fix looks promising. I tried without expanding modular pipelines and the nodes seem to position similarly. However, upon expanding modular pipelines, there seems to be some random positioning like below - Run 1 - Run 2 - Observation: Steps to reproduce:
Some questions:
Thank you |
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really good @rashidakanchwala , tested few times and the order seems the same. Great work !! 💯
Description
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
andfeature
in the example he shared. This issue stemmed from thelayer_dependencies
dictionary inservices/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 intoposort
.For
nodes
andedges
, 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 reorderednodes
andedges
. I added tests innormalise-data-test
andactions/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.QA notes
make build
pip install -e package
kedro viz --autoreload
Make changes to the files, doc changes, and see the layout is consistent.
Checklist
RELEASE.md
file