-
Notifications
You must be signed in to change notification settings - Fork 927
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
Add docs on using namespaces at the node level for grouping nodes. #4518
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Dmitry Sorokin <[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.
The section by itself looks really good, but it seems a bit separate from the rest of the page. I don't think it's super clear here why the first part is for re-use and the second part only for grouping. I don't think they're mutually exclusive and you could argue that namespacing pipelines is also a way of grouping.
Maybe it's not necessary to address all of that in this PR, but at the very least I'd add something at the top of the page to explain that it has two sections: 1. Reusing pipelines with namespaces 2. Grouping nodes with namespaces
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.
Thanks for this PR @DimedS . I have some questions as well, sorry for not giving more specific advice on how to improve this.
|
||
Additionally, Kedro-Viz will allow you to expand and collapse your namespace for better visualisation. | ||
|
||
> **Caution:** While this functionality is useful for grouping nodes easily without additional complexities that may arise when creating a fully namespaced pipeline, be aware that it can create a namespace that is **non-executable**. This happens if you group, for example, the first and last nodes of a pipeline without including the intermediate nodes in the same namespace. In such cases, attempting to execute that namespace with `kedro run --namespace=<your_namespace>` will result in an error, and your visualisation in Kedro-Viz will be broken. |
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.
That sounds a bit scary and makes me think why would the user want to use kedro run --namespace=node_group
instead of kedro run --tags=node_group
.
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 functionality is quite similar - using a namespace at the node level simply adds a prefix to the node name. However, the key benefit I see is the enhanced visualisation in Kedro-Viz.
Signed-off-by: Dmitry Sorokin <[email protected]>
Thanks for the reviews, @merelcht and @astrojuanlu ! I hope I’ve addressed your comments - requesting a re-review. |
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.
Approved with 1 nitpick 👍🏼
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]> Signed-off-by: Dmitry Sorokin <[email protected]>
|
||
Additionally, Kedro-Viz will allow you to expand and collapse your namespace for better visualisation. | ||
|
||
> **Caution:** While this functionality is useful for grouping nodes easily without additional complexities that may arise when creating a fully namespaced pipeline, be aware that it can create a namespace that is **non-executable**. This happens if you group, for example, the first and last nodes of a pipeline without including the intermediate nodes in the same namespace. In such cases, attempting to execute that namespace with `kedro run --namespace=<your_namespace>` will result in an error, and your visualisation in Kedro-Viz will be broken. |
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.
Can this part be in a warning notation: like this
```{warning} |
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.
There's some more sections in this page which are not modified in this PR which are formatted like this which could be formatted as ```{note} sections?
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.
Thanks @ankatiyar , I changed them all, looks really much better!
Overall looks good! Side note: I'm wondering if this should be documented just yet? Should we wait for what comes out of namespace validation exercise or maybe it's not a big deal because we can always update this page? |
Signed-off-by: Dmitry Sorokin <[email protected]>
Thanks for the review, @ankatiyar! I think it's best to merge it now since the functionality is already in place, and tomorrow @Huongg will highlight it in the Coffee Chat. It will be merged anyway but published to stable only after the next release, so we’ll have time to implement additional validation checks before the docs become stable. |
Description
This PR expands the namespace documentation. Previously, we explained how to use namespaces for pipeline reusability. This update extends the manual to demonstrate how creating namespaces at the node level can be useful for grouping nodes.
This PR is related to #4441.
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
RELEASE.md
file