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

Added new docs section that warns against using dot notation for pipe… #3085

Merged
merged 11 commits into from
Sep 27, 2023

Conversation

MehdiNV
Copy link
Contributor

@MehdiNV MehdiNV commented Sep 26, 2023

Description

Related to kedro-org/kedro-viz#1522

There is no warning given to users, in code or docs, from my understanding that advises them to avoid using . notation (which ordinarily indicates namespace pipeline). If . is used, it leads to disconnected and erratic behaving Kedro structures.

Development notes

  • Added text in the md file, advising users to not add . when creating pipelines (and alternatively suggesting _)

QA notes

Documentation website should now have a new section at the bottom, warning users against creating pipelines w/ dot notation.

Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

Looks good to me but a few minor changes requested

Signed-off-by: Mehdi Naderi Varandi <[email protected]>
@MehdiNV
Copy link
Contributor Author

MehdiNV commented Sep 27, 2023

Looks good to me but a few minor changes requested

Good call! Have just committed new changes up

It's only the dot notation that breaks Kedro, other characters like ()!@£ etc should be completely fine. Is it better to be specific and say "Don't use dot notation" or keep it a bit expansive like it is now and say "Don't use improper characters like dot notation"?

@stichbury
Copy link
Contributor

It's only the dot notation that breaks Kedro, other characters like ()!@£ etc should be completely fine. Is it better to be specific and say "Don't use dot notation" or keep it a bit expansive like it is now and say "Don't use improper characters like dot notation"?

I think if it's only the dot notation, then you should be specific about that, rather than restrict the reader (prevent them using other characters) unnecessarily.

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.

Wondering if this could be made shorter and turned into a warning instead of a whole section of its own, something like

````warning
Kedro treats dots `.` in input and output names as namespace separators. If you do not want to use namespaces, avoid the use of dots in your input and output names, and use underscores `_` or other separators instead.

Example:

```python
pipeline([
  node(
    func=lambda x: x,
    inputs="input1kedro",
    outputs="outputs1.kedro",  # Will assume a namespace that doesn't exist!
  )
])
```
````

@stichbury
Copy link
Contributor

I know where you're coming from as I thought the same initially. However, if you look in the context of the rest of that section, it fits quite well as an individual entry and if we modified this one to make it a warning, we should probably do the same with the others (and then it would look weird). So IMO it's OK as it is, in this case.

@astrojuanlu
Copy link
Member

Maybe put it under a "Troubleshooting" section then?

@MehdiNV
Copy link
Contributor Author

MehdiNV commented Sep 27, 2023

Maybe put it under a "Troubleshooting" section then?

@stichbury @astrojuanlu I'm personally fine w/ either, is there a preference? Docs is not my strong suit so not sure which way to personally lean myself

I assume with the Troubleshooting approach, we'd just create a new page named 'Troubleshooting' and move some of these texts there instead?

Signed-off-by: Mehdi Naderi Varandi <[email protected]>
@stichbury
Copy link
Contributor

I assume with the Troubleshooting approach, we'd just create a new page named 'Troubleshooting' and move some of these texts there instead?

Probably not, we don't need a page with a generic name because readers won't know what it contains. It's fine as it is -- I don't think we need to rename the section you've included it under. I'm happy with this now, so approving.

@stichbury stichbury self-requested a review September 27, 2023 16:12
@MehdiNV MehdiNV merged commit 319a8e7 into main Sep 27, 2023
@MehdiNV MehdiNV deleted the docs/add-dot-notation-warning branch September 27, 2023 17:18
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