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

docs/nodes-grouping #4519

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

docs/nodes-grouping #4519

wants to merge 13 commits into from

Conversation

Huongg
Copy link
Contributor

@Huongg Huongg commented Feb 25, 2025

Description

The first draft for nodes grouping documentation. Content is TBC by the team.
closes #4441

Development notes

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Signed-off-by: Huong Nguyen <[email protected]>
@DimedS
Copy link
Member

DimedS commented Feb 26, 2025

Thanks for the PR, @Huongg - great job! Here are a few initial comments:

  • The table is a bit hard to read, possibly due to short line spacing.
  • The images are too small and not clickable, making it difficult to see details.
  • Some table entries contain too much text. We might consider shortening them by keeping only the key points where necessary.
Screenshot 2025-02-26 at 10 49 01

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 general content looks good to me, but I'm not sure the table is the right format. It's quite small and the bullet points aren't formatted nicely inside the columns. Perhaps you can just write the content inside paragraphs with clear headings?

I've also left some comments on the phrasing, I find the tone of the text a bit too marketing style, like we're trying to sell this feature instead of explaining it. I'd suggest changing the tone to make it more informative and explain the why more than the what.

@Huongg Huongg force-pushed the docs/nodes-grouping branch from 7fae928 to 53dae3c Compare February 27, 2025 14:58
Huong Nguyen added 4 commits February 27, 2025 15:13
Signed-off-by: Huong Nguyen <[email protected]>
Signed-off-by: Huong Nguyen <[email protected]>
Signed-off-by: Huong Nguyen <[email protected]>
@Huongg
Copy link
Contributor Author

Huongg commented Feb 27, 2025

hey @DmitrySorokinQB and @merelcht thanks for reviewing this PR! I agree that the table format was quite hard to read in this form. I've updated it to use paragraphs instead, but I've also included a summary table at the end to highlight the key points and make it easier for users.

@Huongg Huongg marked this pull request as ready for review February 27, 2025 19:23
@Huongg Huongg changed the title [DRAFT] docs/nodes-grouping docs/nodes-grouping Feb 27, 2025
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Huongg and team! Left a few comments.

More broadly, the "what works in Kedro - what doesn't work in Kedro - how to use" section titles feel a bit repetitive, but the "best used when - not to use when" are useful. Maybe when we're settled on the content we can turn some of these bullet lists into prose?

@@ -36,6 +36,11 @@ This following pages provide information for deployment to, or integration with,
* [Prefect](prefect.md)
* [Vertex AI](vertexai.md)


# Effective node grouping for deployment
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Effective node grouping for deployment
## Effective node grouping for deployment

There's something funky happening with the navigation bar with these pages 🤔

Compare https://docs.kedro.org/en/nightly/

image

with this PR

image

maybe turning this into an H2 will fix it

Comment on lines 44 to 45
``` {warning}
We also have legacy documentation pages for the following deployment targets, but these have not been tested against recent Kedro releases and we cannot guarantee them:
Copy link
Member

Choose a reason for hiding this comment

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

We might want to move this warning before the "effective node grouping" section

![Switching between different pipelines in Kedro Viz](../meta/images/kedro_viz_switching_pipeline.gif)

### What doesn't work with Kedro
- If you need to create custom node groupings that are different with your existing pipelines, creating new pipelines for them may not be convenient. Instead, you can use alternative grouping methods such as tags or namespaces.
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand this sentence 🤔

### What doesn't work with Kedro
- If you need to create custom node groupings that are different with your existing pipelines, creating new pipelines for them may not be convenient. Instead, you can use alternative grouping methods such as tags or namespaces.
- You cannot execute more than one pipeline in a single step because the kedro run --pipeline command allows running one pipeline at a time.
- You can switch between different pipelines in Kedro Viz, but the visualisation does not support collapsing or expanding pipelines within Kedro Viz.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- You can switch between different pipelines in Kedro Viz, but the visualisation does not support collapsing or expanding pipelines within Kedro Viz.
- You can switch between different pipelines in Kedro Viz, but the flowchart view does not support collapsing or expanding pipelines.

?


### What doesn't work with Kedro
- Nodes with the same tag can exist in different pipelines, making debugging and maintaining the codebase more challenging.
- Executing more than one tag in a single step isn't always possible, and sometimes it will fail to run if a part of pipeline is not populated before at the time of execution.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this part of every method though? For example, if I run kedro run --pipeline data_science before data_processing, it will fail, would it not?

Comment on lines +55 to +56
- Tags are not hierarchical, so tracking groups of nodes can become difficult.
- Tags do not enforce structure like pipelines or namespaces.
Copy link
Member

Choose a reason for hiding this comment

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

These two are they key ones I think 💯

### What works with Kedro
- Namespaces allow you to group nodes, ensuring clear dependencies and separation within a pipeline while maintaining a consistent structure.
- As the same as pipelines or tags, you can enable selective execution using namespaces.
- Namespaces improve visualisation in Kedro-Viz.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "Kedro Viz supports expand and collapsing namespaces as nested pipelines" or sth like that?

- **Defining namespace at Pipeline-level:** When applying a namespace at the pipeline level, Kedro automatically renames all inputs, outputs, and parameters within that pipeline. This can introduce naming conflicts.

### Best used when
- You want to organise nodes logically within a pipeline while keeping a structured execution flow.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could mention that namespaces allow nesting?

@astrojuanlu
Copy link
Member

Huongg and others added 5 commits February 28, 2025 16:04
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Huong Nguyen <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Huong Nguyen <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Signed-off-by: Huong Nguyen <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Signed-off-by: Huong Nguyen <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Signed-off-by: Huong Nguyen <[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.

Node grouping guide for deployment
4 participants