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.

Sign up for GitHub

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

Fix incorrect data node rendering for modular pipelines #1439

Merged
merged 11 commits into from
Jul 25, 2023
Merged

Fix incorrect data node rendering for modular pipelines #1439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a66d1dc
fixed
rashidakanchwala Jul 10, 2023
889b49e
revert other changes
rashidakanchwala Jul 10, 2023
a8b7a70
revert other changes
rashidakanchwala Jul 10, 2023
e506b60
fix after chat with Ravi
rashidakanchwala Jul 10, 2023
31ef883
fixed
rashidakanchwala Jul 21, 2023
fa4b938
Merge branch 'main' into fix-incorrect-data-node-rendering
rashidakanchwala Jul 23, 2023
bd7f6d4
fix lint
rashidakanchwala Jul 23, 2023
7ba5838
fix tests
rashidakanchwala Jul 23, 2023
fe181b2
add release notes
rashidakanchwala Jul 24, 2023
ea6a2b3
Merge branch 'main' into fix-incorrect-data-node-rendering
tynandebold Jul 25, 2023
41cfbe1
Update RELEASE.md
rashidakanchwala Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions RELEASE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ Please follow the established format:
- Include the ID number for the related PR (or PRs) in parentheses
-->


## Bug fixes and other changes

- Fix incorrect rendering of datasets in modular pipelines. (#1439)

# Release 6.3.4

## Bug fixes and other changes
Expand Down
16 changes: 4 additions & 12 deletions package/kedro_viz/data_access/managers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ def create_modular_pipelines_tree_for_registered_pipeline(
modular_pipelines_tree = modular_pipelines_services.expand_tree(
modular_pipelines.as_dict()
)
root_children_ids = set()
root_parameters = set()

# turn all modular pipelines in the tree into a graph node for visualisation,
# except for the artificial root node
Expand All @@ -397,32 +397,24 @@ def create_modular_pipelines_tree_for_registered_pipeline(
)

# only keep the modular pipeline's inputs belonging to the current registered pipeline
inputs_in_registered_pipeline = set()
for input_id in modular_pipeline_node.inputs:
input_node = self.nodes.get_node_by_id(input_id)
if input_node.belongs_to_pipeline(registered_pipeline_id):
edges.add_edge(
GraphEdge(source=input_id, target=modular_pipeline_id)
)
node_dependencies[input_id].add(modular_pipeline_id)
inputs_in_registered_pipeline.add(input_id)
root_children_ids.update(
modular_pipeline_node.external_inputs & inputs_in_registered_pipeline
)
if isinstance(input_node, ParametersNode):
root_parameters.add(input_id)

# only keep the modular pipeline's outputs belonging to the current registered pipeline
outputs_in_registered_pipeline = set()
for output_id in modular_pipeline_node.outputs:
output_node = self.nodes.get_node_by_id(output_id)
if output_node.belongs_to_pipeline(registered_pipeline_id):
edges.add_edge(
GraphEdge(source=modular_pipeline_id, target=output_id)
)
node_dependencies[modular_pipeline_id].add(output_id)
outputs_in_registered_pipeline.add(output_id)
root_children_ids.update(
modular_pipeline_node.external_outputs & outputs_in_registered_pipeline
)

# After adding modular pipeline nodes into the graph,
# There is a chance that the graph with these nodes contains cycles if
Expand Down Expand Up @@ -456,7 +448,7 @@ def create_modular_pipelines_tree_for_registered_pipeline(
or not node.belongs_to_pipeline(registered_pipeline_id)
):
continue
if not node.modular_pipelines or node_id in root_children_ids:
if not node.modular_pipelines or node_id in root_parameters:
modular_pipelines_tree[ROOT_MODULAR_PIPELINE_ID].children.add(
ModularPipelineChild(
node_id, self.nodes.get_node_by_id(node_id).type
Expand Down
1 change: 1 addition & 0 deletions package/tests/test_api/test_rest/test_responses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ def assert_example_data(response_data):
"children": [
{"id": "0ecea0de", "type": "data"},
{"id": "f1f1425b", "type": "parameters"},
{"id": "f0ebef01", "type": "parameters"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So was part of the problem that the expected_modular_pipelines in this test wasn't actually correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part that is weird.
In unit tests parameters that are within a modular pipeline were not showing up in 'root' but in the demo project they were. And as discussed design wise this was what was decided and also made sense. So I had to update this unit test to not only include paramters that are at top-level (f1f1425b) but also parameters that are part of a modular pipeline (f0ebef01)

{"id": "uk", "type": "modularPipeline"},
],
"id": "__root__",
Expand Down