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.

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

Conversation

rashidakanchwala
Copy link
Contributor

@rashidakanchwala rashidakanchwala commented Jul 10, 2023

Description

Fixes #1123

Development notes

The viz flowchart is actually a representation of modular pipelines. The root modular pipeline is the first thing we see in the collapsed flowchart. It must include:

  • All top-level modular pipelines (e.g., level1.node, not level2.level1.node).
  • Any tasks, datasets, or parameters not part of a modular pipeline.
  • Inputs and outputs to the top-level modular pipeline (also called external_inputs and external_outputs in the code).
  • Lastly, all parameters, even if they belong to nested modular pipelines. The flowchart design ensures these parameters remain visible from the root modular pipeline, even when we expand the view.

In the code, there was a variable named root_children_ids, which was causing issues as reported by users. It's a bit confusing what it does but removing it resulted in the parameters not being shown at the root level anymore.

To address this, the variable root_children_ids has been renamed to root_parameters, and it has been modified to include all parameters that are part of a modular pipeline.

After making this change, the code correctly satisfies the above four conditions, ensuring all relevant components are present in the root modular pipeline.

The dummy project the original user posted in the original issue available on box. Please ping me for the link to it

QA notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@rashidakanchwala rashidakanchwala marked this pull request as draft July 11, 2023 09:51
@rashidakanchwala rashidakanchwala marked this pull request as ready for review July 23, 2023 20:15
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

The code looks good to me 👍 Maybe add this to the release notes?

@@ -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)

Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Code looks good.

I couldn't actually test the UI locally with the basic_spaceflights project you provided, as I keep getting this warning even after installing your local branch of the package.

WARNING: You are using an old version of Kedro Viz. You are using version 4.7.1; however, version 6.3.4 is now available.

If you can provide further clarity on how to test that would be great.

RELEASE.md Outdated Show resolved Hide resolved
@tynandebold tynandebold removed the request for review from yetudada July 25, 2023 10:58
Co-authored-by: Tynan DeBold <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate incorrect rendering of dataset node seen on a project with nested modular pipelines
4 participants