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

Breakdown flowchart models into separate files #2144

Merged
merged 19 commits into from
Oct 30, 2024
Merged

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Oct 17, 2024

Description

Related to: #1909

This PR refactors the flowchart.py module by breaking down its contents into separate, more maintainable files within the kedro_viz.models.

What was done:

  • Separating classes and utilities into logically grouped modules.

Development notes

nodes.py: Contains node related classes such as GraphNode, TaskNode, DataNode, TranscodedDataNode, ParametersNode, and ModularPipelineNode.

node_metadata.py: Contains metadata classes for nodes, including GraphNodeMetadata, TaskNodeMetadata, DataNodeMetadata, TranscodedDataNodeMetadata, and ParametersNodeMetadata.

edges.py: Contains the GraphEdge class.

pipelines.py: Contains pipeline-related classes such as RegisteredPipeline and ModularPipelineNode.

tag.py: Contains the Tag class.

model_utils.py: Contains shared utilities and base classes like NamedEntity, GraphNodeType, ModularPipelineChild, and utility functions (_parse_filepath, _extract_wrapped_func).

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

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
@SajidAlamQB SajidAlamQB marked this pull request as ready for review October 25, 2024 09:54
@SajidAlamQB SajidAlamQB self-assigned this Oct 25, 2024
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

Hey @SajidAlamQB , thanks for the PR and for taking the time to walk me through your changes earlier. The new structure looks great and is quite easy to follow. I do have a few observations that took me a bit to fully understand. It seems there are foundational classes, or base classes, that other node classes (like TaskNode) inherit from, such as GraphNodeMetadata and GraphNode. I was thinking it might make sense to introduce a base.py file to separate these foundational classes from their derived ones.

This approach could bring a few benefits:

  1. Separation of Concerns: Clearly distinguishing base classes from the other classes that inherit improves clarity around what’s foundational versus specialized.
  2. Readability and Maintainability: It would make it easier to locate and understand the main functionality provided by the base classes.

@rashidakanchwala Do you think this structure would work with our codebase? My knowledge with the back-end is somewhat limited, so please let me know if this approach wouldn’t be a good fit for our application.

Signed-off-by: Sajid Alam <[email protected]>
@rashidakanchwala
Copy link
Contributor

Hey @SajidAlamQB , thanks for the PR and for taking the time to walk me through your changes earlier. The new structure looks great and is quite easy to follow. I do have a few observations that took me a bit to fully understand. It seems there are foundational classes, or base classes, that other node classes (like TaskNode) inherit from, such as GraphNodeMetadata and NodeGraph. I was thinking it might make sense to introduce a base.py file to separate these foundational classes from their derived ones.

This approach could bring a few benefits:

  1. Separation of Concerns: Clearly distinguishing base classes from the other classes that inherit improves clarity around what’s foundational versus specialized.
  2. Readability and Maintainability: It would make it easier to locate and understand the main functionality provided by the base classes.

@rashidakanchwala Do you think this structure would work with our codebase? My knowledge with the back-end is somewhat limited, so please let me know if this approach wouldn’t be a good fit for our application.

Hey @huong, thanks for sharing your thoughts! I agree that separating foundational classes can sometimes enhance readability, but for now, I think our current setup works well.

Since GraphNodes is in nodes.py along with its children classes and GraphNodeMetadata is in nodes_metadata.py with all NodeMetadata subclasses, this structure already keeps related classes together, which feels cohesive and reduces extra navigation.



class RegisteredPipeline(NamedEntity):
"""Represent a registered pipeline in a Kedro project."""
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about this file but I know it aligns the way we have broken things down. Let's keep it as is for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel the same not sure if its best placed on its own like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I liked your original way of doing it, calling it NamedEntity.py , and having all three classes in there (NamedEntity, RegisteredPipeline and Tags). I know NamedEntity is not clear by just reading the filename, but with the definition it gives clarity. Should we go back to before ? wdyt ?

Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

LGTM. thanks a lot @SajidAlamQB

nit - can we have a test_flowchart folder, it's just we do our test folder structure exact replica of our src folder structure.

Signed-off-by: Sajid Alam <[email protected]>
Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

the test folder changes look good to me, thanks @SajidAlamQB

Signed-off-by: Sajid Alam <[email protected]>
@SajidAlamQB SajidAlamQB merged commit 4db2bf9 into main Oct 30, 2024
34 checks passed
@SajidAlamQB SajidAlamQB deleted the breakdown-flowchart branch October 30, 2024 11:15
@Huongg Huongg mentioned this pull request Nov 21, 2024
5 tasks
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.

4 participants